Skip to content
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

Open
jvmap opened this issue Jan 29, 2025 · 11 comments · May be fixed by #16327
Open

Unclear warning use-safe-access: Use the safe access (.?) operator. #16239

jvmap opened this issue Jan 29, 2025 · 11 comments · May be fixed by #16327

Comments

@jvmap
Copy link

jvmap commented Jan 29, 2025

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:

  1. Unspecific error message. It should indicate which of the three '.' operators on that line are a problem;
  2. I found out, by trial and error, that changing the last '.' operator to '.?' makes the warning go away. However, now my Bicep file is objectively worse, since I've lost type checking on the 'delegatedManagedIdentityResourceId' property.

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

@jeskew
Copy link
Member

jeskew commented Jan 29, 2025

The diagnostic includes a "quick fix" (Ctrl + . in VS Code to trigger) that will insert a .? at the right spot. Diagnostics are always targeted to a specific piece of syntax, which in this case was <base expression>.delegatedManagedIdentityResourceId. I take your point that it can be confusing if the base expression also contains property accesses, but diagnostics targeting a property or array access will always be talking about the outermost access in the underlined code.

For reference, compound accesses are parsed in this way:

webApp.outputs.identity.delegatedManagedIdentityResourceId
++++++
~~~~~~~~~~~~~~
-----------------------
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The syntax underlined by +++ is the "base expression" (the thing whose property is being dereferenced) in the syntax underlined by ~~~, which is in turn the base expression of the syntax underlined by ---, which is in turn the base expression of the syntax underlined by ^^^.

Could you clarify what type checking is missing from the syntax using the .? operator? resourceInput<'Microsoft.Authorization/roleAssignments@2022-04-01'>.properties.delegatedManagedIdentityResourceId accepts a value of type null | string, which is what webApp.outputs.identity.?delegatedManagedIdentityResourceId resolves to. webApp.outputs.identity.delegatedManagedIdentityResourceId had a hidden error condition that is now being detected: webApp.outputs.identity's type allowed a value of {principalId: '...'}, which would have caused the deployment to fail.

@jvmap
Copy link
Author

jvmap commented Jan 29, 2025

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.
In the original code:
delegatedManagedIdentityResourceId: webApp.outputs.identity.delegatedManagedIdentityResourceId

The name of the property delegatedManagedIdentityResourceId (on the right-hand side) is checked against the declared outputs of the webapp.bicep module (the ManagedServiceIdentity type). Now, if I apply the quick fix, the code becomes:
delegatedManagedIdentityResourceId: webApp.outputs.identity.?delegatedManagedIdentityResourceId

Now, a typo could easily sneak in here. For example, Bicep will be perfectly happy with:
delegatedManagedIdentityResourceId: webApp.outputs.identity.?typoDelegatedManagedIdentityResourceId

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:

@description('The web app identity object.')
output identity ManagedServiceIdentity = {
  principalId: webApp.identity.principalId
}

and this behaves differently from

@description('The web app identity object.')
output identity ManagedServiceIdentity = {
  principalId: webApp.identity.principalId
  delegatedManagedIdentityResourceId: null
}

The first case fails, during deployment, with the error:
'The language expression property 'delegatedManagedIdentityResourceId' doesn't exist, available properties are 'principalId'.
I think this is the 'hidden error condition' you talked about.

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):

@description('The web app identity object.')
output identity ManagedServiceIdentity = {
  principalId: webApp.identity.principalId
}

Bicep should warn that the property delegatedManagedIdentityResourceId is not set.

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.

@jeskew
Copy link
Member

jeskew commented Jan 29, 2025

There should maybe be an informational or warning diagnostic on webApp.outputs.identity.?typoDelegatedManagedIdentityResourceId. It's not there today because given the type:

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. ManagedServiceIdentity isn't a sealed type, so it can have additional properties. To change this, use the @sealed() decorator:

@sealed()
type ManagedServiceIdentity = {
  principalId: string
  delegatedManagedIdentityResourceId: string?
}

This will also cause webApp.outputs.identity.?typoDelegatedManagedIdentityResourceId to raise a warning diagnostic since the compiler knows that there will never be a property named typoDelegatedManagedIdentityResourceId on webApp.outputs.identity

@jvmap
Copy link
Author

jvmap commented Jan 30, 2025

Thank you! Adding @sealed() to the ManagedServiceIdentity type achieved my goal. Now the safe access operator can be used without losing type-checking on the properties of my custom type.

It's good to know more about the intricacies of the Bicep/ARM type system. To me, it is quite surprising that the value

{
  principalId: webApp.identity.principalId
}

matches the type

type ManagedServiceIdentity = {
  principalId: string
  delegatedManagedIdentityResourceId: string?
}

despite not declaring a value for the delegatedManagedIdentityResourceId member, and does not equal this value:

{
  principalId: webApp.identity.principalId
  delegatedManagedIdentityResourceId: null
}

When working with custom types, this design choice could lead to a lot of confusion and subtle bugs.

@jvmap
Copy link
Author

jvmap commented Jan 30, 2025

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 x matches type T, value x might have the properties defined by type T, or value x might not have any of the properties defined by type T, or value x might have properties that are not defined by type T. In other words, even if x matches type T, the actual properties of x could be completely different from the properties defined by type T.

It seems that the only guarantee is that when x has a primitive property that is defined in T, say property p, then the primitive value of p (x.p) must have the primitive type defined in T. For example, you cannot assign an integer to a string property.

@jeskew
Copy link
Member

jeskew commented Jan 30, 2025

when value x matches type T, value x might have the properties defined by type T, or value x might not have any of the properties defined by type T, or value x might have properties that are not defined by type T. In other words, even if x matches type T, the actual properties of x could be completely different from the properties defined by type T.

That's not quite right. I think the main thing causing confusion here is that Bicep does not distinguish between null and undefined. Only properties with a nullable type may be omitted. In other words,

{
  principalId: webApp.identity.principalId
}

matches the type

type ManagedServiceIdentity = {
  principalId: string
  delegatedManagedIdentityResourceId: string?
}

but

{
  delegatedManagedIdentityResourceId: 'id'
}

would not.

Wrt the mention of primitive types: no type is nullable by default but must be explicitly declared as such with a ?. You can't omit or supply null for a target expecting an object or an array -- object? != object, and ManagedServiceIdentity? != ManagedServiceIdentity.

@jvmap
Copy link
Author

jvmap commented Jan 30, 2025

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: null (setting the property to null explicitly) and undefined (omitting the property).
A required property, per my understanding, can be neither null nor undefined.

The values null and undefined behave differently in certain circumstances. This might be inherent in ARM, I'm not sure.

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 null instead of undefined?

So this:

output identity ManagedServiceIdentity = {
  principalId: webApp.identity.principalId
}

would implicitly be equal to:

output identity ManagedServiceIdentity = {
  principalId: webApp.identity.principalId
  delegatedManagedIdentityResourceId: null
}

At least then all the properties of a type are always defined (but can still be null in case of a nullable property).

Alternatively, it may be worth a warning when omitting a nullable property from a value, since the actual property value becomes undefined, whereas a typical user expectation could be that the property value is null.

@jeskew
Copy link
Member

jeskew commented Jan 30, 2025

The main constraint that Bicep is working with here is that Azure resource providers, by design, do not distinguish between omitted and null-valued properties. If a property is optional on a response for a resource declaration, sometimes it'll be null, and sometimes the property won't be there. As much as possible, we don't want values to behave differently based on their provenance.

Maybe, if a nullable property is omitted from a value, that property should implicitly get the value null instead of undefined?

There's more discussion about this in #387. This distinction is important in ARM JSON templates, where the The language expression property 'delegatedManagedIdentityResourceId' doesn't exist, available properties are 'principalId'. deployment failure message is the only guard you have against mistyping a property name.

@jvmap
Copy link
Author

jvmap commented Jan 31, 2025

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:

output identity ManagedServiceIdentity = {
  principalId: webApp.identity.principalId
}

and this is "fine", no warning emitted, even though it behaves differently from this value:

output identity ManagedServiceIdentity = {
  principalId: webApp.identity.principalId
  delegatedManagedIdentityResourceId: null
}

The first value could generate the error The language expression property 'delegatedManagedIdentityResourceId' doesn't exist, available properties are 'principalId'., whereas the second value will never do that.

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.

@jeskew
Copy link
Member

jeskew commented Feb 3, 2025

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 null value as equivalent to a property being omitted.

Taking a step back, what would be your ideal solution here? Would it be a diagnostic webApp.outputs.identity.?typoDelegatedManagedIdentityResourceId, or would it be for Bicep to surface a distinction between null-valued and omitted properties?

@jvmap
Copy link
Author

jvmap commented Feb 4, 2025

Hi @jeskew . Thanks for clarifying. I better understand now where Bicep is coming from.

Basically, Bicep does not want to make a distinction between null and an omitted property, but ARM does. This is why the values

output identity ManagedServiceIdentity = {
  principalId: webApp.identity.principalId
}

and

output identity ManagedServiceIdentity = {
  principalId: webApp.identity.principalId
  delegatedManagedIdentityResourceId: null
}

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 null and an omitted property is appropriate, at least in some cases. For example, the error:

The language expression property 'delegatedManagedIdentityResourceId' doesn't exist, available properties are 'principalId'.

is actually useful and much better than silently treating delegatedManagedIdentityResourceId as being equal to null.

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 null. In such a world, webApp.outputs.identity.delegatedManagedIdentityResourceId would not be an unsafe access: delegatedManagedIdentityResourceId would either have a value or be null - the existence of the property is guaranteed by the language. So, that would solve all the problems and confusion in this scenario. There would be no warning and no need to "fix" a Bicep file that does not have a demonstrable problem to begin with.

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 .? and @sealed, because:

  1. .? communicates the wrong intent, because the existence of this property is not optional (although its value could be null). All of this known at compile-time, there is nothing unsafe about the access. It only becomes "unsafe" because Bicep considers the existence of a property that I explicitly define as unknown. ARM does not have this problem, it is purely a Bicep oddity.
  2. @sealed communicates the wrong intent, because I don't need or care for the type to be sealed, I'm just working around the (in my view) inappropriate use of .? and its implications for compile-time type checking. In other words, using or not using @sealed should be an orthogonal decision and not required for static type checking of types that have one or more nullable properties.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

3 participants