-
Notifications
You must be signed in to change notification settings - Fork 763
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
Unclear warning use-safe-access: Use the safe access (.?) operator. #16239
Comments
The diagnostic includes a "quick fix" (Ctrl + . in VS Code to trigger) that will insert a For reference, compound accesses are parsed in this way:
The syntax underlined by Could you clarify what type checking is missing from the syntax using the |
Hi @jeskew . Thanks for the quick reply. My Visual Studio Code Bicep extension and Azure CLI had gotten out-of-sync, so I did not have the warning or the quick fix in Visual Studio Code. However, after updating the extension in Visual Studio Code, I now indeed see the quick fix. This is certainly a better experience than trial-and-error, thanks! However, I still think the quick fix makes the Bicep template worse. This relates to the missing type checking. The name of the property Now, a typo could easily sneak in here. For example, Bicep will be perfectly happy with: This is what I meant with type checking. To make matters worse, the above line would work perfectly well when deploying within the same tenant, but there would be missing permissions in a cross-tenant deployment (this is unrelated to Bicep). I want to detect such an error as soon as possible, hence my need for strict type checking. With regards to the "hidden error condition" that is now detected. I see that it is indeed possible, in webapp.bicep, to define the output as:
and this behaves differently from
The first case fails, during deployment, with the error: The second case does not produce an error. So I kind of get what the warning is trying to help with, but solving this warning introduces new, worse error conditions (the typo problem I mentioned above). I would much rather have a failed deployment than an incorrect "successful" deployment. To me, it seems that the real problem here is that it's possible to output a value (in webapp.bicep) that does not have the fields that are explicitly declared in the type (ManagedServiceIdentity). This code should definitely yield a warning (but doesn't):
Bicep should warn that the property The line that actually produces the warning does not have any problem with it. Applying the quick-fix basically means giving up type checking altogether. This is certainly not an improvement in my case. |
There should maybe be an informational or warning diagnostic on type ManagedServiceIdentity = {
principalId: string
delegatedManagedIdentityResourceId: string?
} The following values are considered a match for the type: {
principalId: webApp.identity.principalId
delegatedManagedIdentityResourceId: webApp.id
}
{
principalId: webApp.identity.principalId
delegatedManagedIdentityResourceId: null
}
{
principalId: webApp.identity.principalId
}
{
principalId: webApp.identity.principalId
delegatedManagedIdentityResourceId: crossTenant ? webApp.id : null
typoDelegatedManagedIdentityResourceId: 'foo'
} Bicep will emit a warning on the last one, but it's a type match as far as ARM is concerned. @sealed()
type ManagedServiceIdentity = {
principalId: string
delegatedManagedIdentityResourceId: string?
} This will also cause |
Thank you! Adding It's good to know more about the intricacies of the Bicep/ARM type system. To me, it is quite surprising that the value
matches the type
despite not declaring a value for the
When working with custom types, this design choice could lead to a lot of confusion and subtle bugs. |
The more I think about Bicep/ARM types, the more confused I get. What does a "type match" mean in ARM? Per my current understanding, when value It seems that the only guarantee is that when |
That's not quite right. I think the main thing causing confusion here is that Bicep does not distinguish between
matches the type
but
would not. Wrt the mention of primitive types: no type is nullable by default but must be explicitly declared as such with a |
Alright, so only nullable properties may be omitted. That at least makes some sense. The tricky part is that making a property nullable instead of required, is that the value space gets extended with two new values: The values The fact that a nullable property is also "undefinable" might be surprising to many users. Maybe, if a nullable property is omitted from a value, that property should implicitly get the value So this:
would implicitly be equal to:
At least then all the properties of a type are always defined (but can still be Alternatively, it may be worth a warning when omitting a nullable property from a value, since the actual property value becomes |
The main constraint that Bicep is working with here is that Azure resource providers, by design, do not distinguish between omitted and
There's more discussion about this in #387. This distinction is important in ARM JSON templates, where the |
Hi @jeskew . Yes, I fully agree that it is not desirable to distinguish between omitted and null-valued properties. That's why I think it's unfortunate, in the current version of Bicep, that you could define this value:
and this is "fine", no warning emitted, even though it behaves differently from this value:
The first value could generate the error So, I belief that the goal (which I support) of "we don't want values to behave differently based on their provenance." is not achieved here. It appears that this in fact a shortcoming (if we should call it that) of Bicep, not of ARM, since, as you say, "Azure resource providers, by design, do not distinguish between omitted and null-valued properties.". Bicep allows nullable properties to be omitted entirely, and thereby introduces the undesirable fact that values behave differently based on their provenance. |
What I meant by "we don't want values to behave differently based on their provenance" is that values of a given type should behave the same whether they are returned by a resource provider or provided as a parameter. If you can make different assumptions about whether a property will be present because it was supplied as a parameter, that would be undesirable from my point of view. To achieve this, Bicep follows ARM's example in allowing nullable properties to be omitted and treating an explicit Taking a step back, what would be your ideal solution here? Would it be a diagnostic |
Hi @jeskew . Thanks for clarifying. I better understand now where Bicep is coming from. Basically, Bicep does not want to make a distinction between
and
behave differently. Bicep thinks these values are identical, but ARM does not agree and treats them differently. The fact that ARM sometimes makes a distinction between
is actually useful and much better than silently treating You write that "Bicep follows ARM's example in allowing nullable properties to be omitted and treating an explicit null value as equivalent to a property being omitted.". In this particular scenario, it would have worked better if Bicep treated a property being omitted the same way as a property explicitly set to If that is not possible, I will have to rewrite my Bicep files to use separate, primitive output variables, instead of custom types. I dislike the current workaround of using
It's not my ideal world, but if Bicep keeps the current behavior, I think I would have to stay away from custom types where possible, so that I can achieve better code quality in my Bicep files. |
Bicep version
Bicep CLI version 0.33.13 (48521b9)
Describe the bug
Since version 0.33.13, the attached bicep files produce the following warning:
webapi.bicep(57,41) : Warning use-safe-access: Use the safe access (.?) operator.
Problems with this warning:
I want this warning to go away, without making my Bicep template worse.
To Reproduce
az bicep lint --file webapi.bicep
Additional context
New warnings are great if they help produce better templates and have a low cost to the user. However, this particular warning suggests a reduction in code quality, whereas the current code is working just fine. It really adds substantial overhead to the use of Bicep when working templates suddenly start spewing warnings that are hard to figure out.
I need the delegatedManagedIdentityResourceId for cross-tenant deployments (Azure Marketplace Managed Application).
webapi.bicep.txt
webapp.bicep.txt
The text was updated successfully, but these errors were encountered: