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

Assigned types should be control-flow sensitive #12121

Open
jeskew opened this issue Oct 11, 2023 · 6 comments
Open

Assigned types should be control-flow sensitive #12121

jeskew opened this issue Oct 11, 2023 · 6 comments
Labels
enhancement New feature or request Needs: Upvote This issue requires more votes to be considered type system

Comments

@jeskew
Copy link
Member

jeskew commented Oct 11, 2023

Is your feature request related to a problem? Please describe.
Within a ternary expression, Bicep can sometimes generate spurious diagnostics when a subexpression within the true or false clause interacts with a symbol that should be safe given the check in the if clause of the ternary.

For example, the following snippet:

var db = {
  name: 'db1'
}

resource postgreSQL 'Microsoft.DBForPostgreSQL/servers@2017-12-01' existing = {
  name: 'name'
  resource database 'database' = {
    name: db.name
    properties: {
      charset: contains(db, 'charset') ? db.charset : 'utf8'
      collation: contains(db, 'collation') ? db.collation : 'English_United States.1252'
    }
  }
}

will generate the following diagnostics:

[BCP053 (Error)] The type "object" does not contain property "charset". Available properties include "name".
[BCP053 (Error)] The type "object" does not contain property "collation". Available properties include "name".

Describe the solution you'd like
Similar to how C# will recognize that foo is non-null inside of a block that starts with if (foo is not null) {, the Bicep compiler's type inference should take an expression's position in control flow into account when assigning a type to an expression. Given that Bicep does not have guard statements that control entry into blocks, this should primarily impact ternary statements. If possible, flow-sensitive typing should take into account:

  • equality operators
  • comparison operators, and
  • some core functions in the sys namespace (e.g., empty, length, contains, endsWith, startsWith, and negations thereof).
@anthony-c-martin
Copy link
Member

Related to #3750 (3rd option)

@anthony-c-martin
Copy link
Member

As suggested in #3750, the following should now be a viable workaround:

var db = {
  name: 'db1'
}

resource postgreSQL 'Microsoft.DBForPostgreSQL/servers@2017-12-01' existing = {
  name: 'name'
  resource database 'database' = {
    name: db.name
    properties: {
      charset: db.?charset ?? 'utf8'
      collation: db.?collation ?? 'English_United States.1252'
    }
  }
}

@jeskew
Copy link
Member Author

jeskew commented Dec 11, 2024

Adding @peter-de-wit's comment from a duplicate issue:

Bicep version
Since 0.27.1

Describe the bug
Since bicep 0.27.1, existing bicep templates cannot be build anymore due errors.
Also unneeded warnings are created in particular cases. The errors and warnings are related to type checking and property existence within objects/arrays.

To Reproduce
Steps to reproduce the behavior:
Use the following template:

var dbs = [
 {
    id: '1'
    name: 'db1'
    properties: {}
 }
 {
    id: '2'
    name: 'db2'
    properties: {}
 }
 {
    id: '3'
    name: 'db3'
    properties: {
      sku: {}
    }
 }
]

var dbsComplete = union(dbs, [
  {
      id: '4'
      name: 'item4'
      properties: {
        prop1: '1'
        sku: {
          tier: 'generalpurpose'
        }
      }
  }
])

var dbsAny = map(dbs, item => any(item))
var vCoreSkus = ['generalpurpose','businesscritical','hyperscale']

// Scenario 1
// Error BCP070: Argument of type "(object | object | object) => error" is not assignable to parameter of type "(any[, int]) => bool".
// Error BCP053: The type "{ sku: object }" does not contain property "prop1". Available properties include "sku".
var result1 = filter(dbs, item => contains(item.properties, 'prop1') && item.properties.prop1 == '1')
var result1Complete = filter(dbsComplete, item => contains(item.properties, 'prop1') && item.properties.prop1 == '1') // does not throw error
var result1Any = filter(dbsAny, item => contains(item.properties, 'value') && item.properties.value == '1') // does not throw error

// Scenario 2
// Error BCP052: The type "object" does not contain property "tier"
var result2 = filter(dbs, item => contains(item.properties, 'obj') && contains(item.properties.sku, 'tier') && contains(vCoreSkus, item.properties.sku.tier))
var result2Complete = filter(dbsComplete, item => contains(item.properties, 'obj') && contains(item.properties.sku, 'item') && contains(vCoreSkus, item.properties.sku.tier)) // does not throw error
var result2Any = filter(dbsAny, item => contains(item.properties, 'obj') && contains(item.properties.obj, 'tier') && contains(vCoreSkus, item.properties.obj.item)) // does not throw error

// Scenario 3
// Warning BCP052: The type "object" does not contain property "tier".
var result3 = filter(dbs, item => contains(vCoreSkus, item.properties.?sku.?tier ?? 'empty'))
var result3Complete = filter(dbsComplete, item => contains(vCoreSkus, item.properties.?sku.?tier ?? 'empty')) // does not throw error
var result3Any = filter(dbsAny, item => contains(vCoreSkus, item.properties.?sku.?tier ?? 'empty')) // does not throw error

// Scenario 4
// ERROR: Error BCP070: Argument of type "(object | object | object) => error" is not assignable to parameter of type "(any[, int]) => bool".
// ERROR: Error BCP052: The type "object" does not contain property "tier".
var result4_pre1 = filter(dbs, item => contains(item.properties, 'sku'))
var result4_pre2 = filter(dbs, item => contains(item.properties.sku, 'tier'))
var result4 = filter(result4_pre2, item => contains(vCoreSkus, item.properties.sku.tier) )

Additional context

  • I would say that when using scenario 3, no warning should be thrown. I choose to use the 'safe dereference operator' so warn me for a possible non-existing property is unnecessary.
  • I would say that encapsuling an object/array through 'any' is not a solution because of the fact that 'correct' build/linting errors are not visible anymore.
  • Did work until bicep v0.26.54

@peter-de-wit
Copy link

peter-de-wit commented Dec 11, 2024

As suggested in #3750, the following should now be a viable workaround:

var db = {
name: 'db1'
}

resource postgreSQL 'Microsoft.DBForPostgreSQL/servers@2017-12-01' existing = {
name: 'name'
resource database 'database' = {
name: db.name
properties: {
charset: db.?charset ?? 'utf8'
collation: db.?collation ?? 'English_United States.1252'
}
}
}

@jeskew thanks for mentioning this issue. For me, above solution is indeed a good workaround when updating existing code is a possibility, but it's not the solution. Existing bicep templates / deployments should not break so suddenly by a minor upgrade (v0.26.54 --> v0.27.1)

Also adding my note within post #15770 :

The compiler is at compiletime aware of the fact that there is a fallback from the expression to 'empty', and that's exactly how the logic is written and intended, so for me the warning is misplaced. I do follow your opinion when the fallback to 'empty' was not there: then I would like to know this and a warning should be thrown.

@daandupau
Copy link

Any update on this issue? We use this feature extensively and cant upgrade to higher biceps features due to this issue

@peter-de-wit
Copy link

peter-de-wit commented Jan 10, 2025

A workarround that does not throw compiler warnings (even when the array is empty) can be accomplished this way:

var vCoreSkus = ['generalpurpose','businesscritical','hyperscale']

// TODO: Since BICEP version 27.1, the compiler has a bug that causes the 'Sku' property to be missing in the 'Properties' object, even though the array is empty.
//       This is a workaround to fix that.
var sqlDatabasesCompilerBugFix = map(dbs, a => union(a, { sku: { Tier: 'None' }} ))
var sqlDatabasesProd = filter(sqlDatabasesCompilerBugFix, resource => resource.IsProductionEnvironment == true)
var sqlDatabasesNonProd = filter(sqlDatabasesCompilerBugFix, resource => resource.IsProductionEnvironment == false)

// Both scenario's now do not throw errors or warnings:
var sqlDatabasesDTUProd = filter(sqlDatabasesProd, resource => contains(vCoreSkus, toLower(resource.Properties.?Sku.?Tier ?? 'None')))
var sqlDatabasesDTUNonProd = filter(sqlDatabasesNonProd, resource => contains(resource.Properties, 'Sku') && !contains(vCoreSkus, toLower(resource.Properties.Sku.Tier)))

I still think that the compiler should not throw a warning in the original situation, and especially when the array is empty after the filter function, no additional warnings/errors should be thrown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Needs: Upvote This issue requires more votes to be considered type system
Projects
Status: Todo
Development

No branches or pull requests

5 participants