Proposal - Better validation for conditional resources/modules

[anthony-c-martin]

Proposal - Better validation for conditional resources/modules

Problem statement

Dealing with conditional resources/modules today can be error prone as you must remember to propagate conditions correctly, and the type system will not alert you to this being done incorrectly - leading to deploy-time errors.

In the following sample, there are no Bicep errors although the output statement may be invalid if someCond is false and the resource does not exist.

param someCond bool

resource resWithCond 'Microsoft.Compute/virtualMachines@2021-03-01' existing = if (someCond) {
  name: 'abc'
}

output myValue string = resWithCond.properties.licenseType

To fix this, the output statement needs to be changed to the following, to propagate the conditional check:

output myValue string = someCond ? resWithCond.properties.licenseType : ''

I've mentioned this idea on a few other issues, but thought it would be a good idea to write it up formally for tracking.

Potential Solutions

Note - these solutions all build on-top of each other - 3 depends on 1 & 2, and 2 depends on 1.

1. Treat conditional references as resource | null

In the first example, if resWithCond has type resource | null, accessing properties directly will raise an error, so the user will be forced to handle the null-ness and write something like the following:

output myValue string = someCond ? resWithCond.properties.licenseType : ''

There are still problems with this approach - it is a little verbose, and Bicep needs to validate that the condition in the ternary is exactly the same as the condition on the resource.

2. Introduce 'null-conditional' operator .?

This builds upon the previous solution, but also adds a new operator which removes the need to propate the someCond condition - so the type error would be fixable with:

output myValue string = resWithCond?.properties.licenseType ?? ''

In the generated JSON output, Bicep would generate a ternary if() statement, bringing the condition for the nullable reference forward:
[if(<someCond>, <resWithCond reference statement>, '')]

This is essentially syntactic sugar for 1., but means that lengthy conditions don't need to be copy-pasted.

3. Going deeper with conditional flow analysis

Again, this builds on 1. & 2., giving Bicep the ability to skip null-checks if the condition has already been checked:

param someCond bool

resource resWithCond 'Microsoft.Compute/virtualMachines@2021-03-01' existing = if (someCond) {
  name: 'abc'
}

resource someOtherRes 'My.Rp/madeUpResource@2021-03-01' = if (someCond) {
  name: 'someOtherRes '
  properties: {
    // the .? and ?? may be skipped because we're inside a block which already has a `if (someCond)` check on it
    someProp: resWithCond.properties.licenseType
  }
}

output myObj object = someCond ? {
  // again, we don't need to handle nullability here because we've already checked for someCond
  someProp: resWithCond.properties.licenseType
} : {}

output myOtherObj object = {
  // we do need to handle nullability here, because we haven't already checked for someCond
  someProp: resWithCond?.properties.licenseType ?? ''
}

Other Notes

1. Any solution for stacking conditions will need to be robust to handle binary bool operations - e.g. inside a block which checks someCond && otherCond - our flow analysis will need to understand that a nullable reference gated on someCond can be used safely, and inside a block which checks someCond || otherCond it cannot be.
2. A solution will also need to be robust enough to perform analysis on assignment through variables - e.g. var combinedCond = someCond && otherCond

[alex-frankel]

I definitely think Option 3 is important regardless of whether we choose Option 1 or Option 2. This assumes we've fixed any greedy conditional evaluation problems in the runtime.

FWIW, I like Option 1 even though it is more verbose, mostly because it is not that much more verbose than Opt.2 and it doesn't introduce any new syntax.

Overall, this will be a great improvement!

[dciborow]

I just hit this issue myself trying to make a set of modules to conditionally add vnet to resources.

Does solution 1 simply prompt the IDE to tell me to fix the issue?

If the fix for the issue is "almost" always, someCond ? resWithCond.properties.licenseType : ''", it would be nicer if the "build" action automatically populations this for me between bicep and arm.

And that would still allow me to customize it if that is not my desired result and I could do something like someCond ? resWithCond.properties.licenseType : defaultLicenseType.

Applying either of the first 2 solutions is leaving me adding boiler plate code to a lot of places.

Even populating "outputs.*" as empty by default would achieve this goal.

[anthony-c-martin]

Does solution 1 simply prompt the IDE to tell me to fix the issue?

Yeah, essentially. It's a bit better than the present-day situation as you don't actually have to submit a validation/deployment to ARM to figure out the issue, but still pretty verbose.

If the fix for the issue is "almost" always, someCond ? resWithCond.properties.licenseType : ''", it would be nicer if the "build" action automatically populations this for me between bicep and arm.

And that would still allow me to customize it if that is not my desired result and I could do something like someCond ? resWithCond.properties.licenseType : defaultLicenseType.

I'm all for shorter syntax, but IMO a problem with picking default behavior without raising any errors/warnings is that it'll come as a surprise to some people who don't expect an empty string to be the default - especially when used in places other than output statements.

Applying either of the first 2 solutions is leaving me adding boiler plate code to a lot of places.

+1 - I wanted to lay out some 'cheaper' solutions to discuss pros/cons, but I would personally want solution 3.

[miqm]

Ultimately I'd go also all the way with no 3, but do no 2 ASAP, as copying conditionals is a pain in you-know-where :)

Additionally, similar to C# we could introduce null-forgiving operator to suppress potential null-usage errors if developer is positive, that this will not cause runtime error (i.e. if the condition is not exactly this same as for resource, but logically does this same). The ! operator would release us from building extremely complex solution to examine logical conditions (especially that they might depend on parameters which we may examine, but we shouldn't make any compilation decisions based on them).

Additionally, in bicepconfig.json we could define if we want to treat nullable usages as warnings or errors.

[majastrz]

When we implement this, let's make sure that #4886 is covered.

[bmoore-msft]

Looking at the linked #1631 - one additional scenario is that the reference() may not be on an output, it may be on a property, or a param (module). Different use case but the fix should be the same - i.e. we don't know what the correct answer is, so we have to ask the developer for help (throw, squiggly, etc).

[cutecycle]

howdy,

Subscribing because I ran into this, I think!

I had a ternary for conditional property defintion and lost lint on both expressions that was there when it was non-conditional.

resource dataFactory 'Microsoft.DataFactory/factories@2018-06-01' = {
  name: dataFactoryName
  location: location
  identity: {
    type: 'SystemAssigned'
  }
  properties: {
    repoConfiguration: (shouldIncludeGitRepositoryConfiguration) ? {
      type: repositoryConifigurationType
      projectName: azureDevOpsProjectName
      accountName: azureDevOpsAccountName
      collaborationBranch: collaborationBranch
      repositoryName: repositoryName
      rootFolder: dataFactoryGitRootFolder
    } : null
  }
}

[tkimmm]

Subscribing because I've also run into this issue

[maartengo]

Just having solution 1 would already be a great help. It quickly becomes hard to spot any missing unchecked conditional references when working on bicep files with lots of conditionals. Getting an error during development would save us from having a broken deployment because we missed it during our tests. It isn't always feasible to test the deployment with all combinations of conditionals.

What is the current progress on this proposal?

[anthony-c-martin]

It feels like we're in a better place to light this up now that we've added both ?? and .?, and have stronger type analysis - @jeskew, @alex-frankel thoughts?

[jeskew]

I think we could tackle this with some slight modifications to code generation: .? will almost always be translated to the ARM tryGet function, whereas here we'd need to convert it to something like if(<condition expression from resource declaration>, reference(...), null()) since we want to avoid evaluating the base expression if the condition is false.

One small note on option 1: I don't think resource | null is the correct type for a conditional resource, since some resource properties will be accessible even if the resource's condition is false, to wit, id, type, apiVersion, and name. I believe this is true whether the values for those properties are resolved by the Bicep compiler or by the ARM resourceInfo function. Instead, the properties property should have a type of <resource body type> | null (and any other top-level properties that get fetched with a 'full' reference expression would also need to be marked as nullable). This is mostly academic, since users would still get a fixable warning on <resource>.properties (or resource.identity, etc.) expressions prompting them to change it to <resource>.?properties or <resource>.properties!.