-
Notifications
You must be signed in to change notification settings - Fork 757
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose fail
function
#15958
base: main
Are you sure you want to change the base?
Expose fail
function
#15958
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/Bicep.Core/Semantics/Namespaces/SystemNamespaceType.cs:1076
- [nitpick] The description for the 'fail' function could be clearer. Consider rephrasing to 'Raises a runtime error with the provided message, causing the deployment to fail when evaluated.'
.WithGenericDescription("Raises a runtime error with the provided message. Will cause a deployment to fail when evaluated.")
src/Bicep.Core.IntegrationTests/ScenarioTests.cs:6604
- [nitpick] The error message 'should have supplied one' is unclear. It should be more descriptive, such as 'A required parameter was not supplied'.
param required string = fail('should have supplied one')
Test this change out locally with the following install scripts (Action run 12521230248) VSCode
Azure CLI
|
Dotnet Test Results 78 files - 39 78 suites - 39 30m 37s ⏱️ - 18m 2s Results for commit 2c339b9. ± Comparison against base commit b732155. This pull request removes 1846 and adds 632 tests. Note that renamed tests count towards both.
|
Resolves #14067
This PR exposes the new
fail
function that is available in the ARM engine. One thing I went back and forth on is what the return type of this function should be. Three viable options are:Error
Any
Never
My first instinct was to use
Error
, but this makes thefail
function surprisingly difficult to use. It's essentially forbidden everywhere unless you wrap it inany()
, which hurts usability. I considered using a return type ofany
to remove the need for this boilerplate -- this is close to the semantic used in the original C# proposal forthrow
expressions -- but I wasn't happy with the downstream effects of given Bicep's union type semantics. Iffail(...)
is of typeany
, thencondition ? 'boo' : fail('boo!')
is of typeany
, since the union ofany
with anything else isany
. This isn't really correct, since the expression will only be evaluated if the condition is true, in which case the ternary's type would be'boo'
.I ended up choosing
never
both because afail(...)
expression can't ever finish evaluating (and therefore doesn't really have a type) and because it had the right downstream assignability behavior. Very open to discussion on this if anyone feels strongly thatnever
is incorrect, though!Microsoft Reviewers: Open in CodeFlow