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

Nullable types #9454

Merged
merged 19 commits into from
Feb 27, 2023
Merged

Nullable types #9454

merged 19 commits into from
Feb 27, 2023

Conversation

jeskew
Copy link
Contributor

@jeskew jeskew commented Jan 5, 2023

Resolves #8973 and resolves #8807

This PR adds a ? type postfix operator that identifies any type expression as nullable, meaning that in addition to values matching the unmodified type, a missing or explicit null value will be accepted.

For example, the following statement will declare a parameter, foo, for which a deployment may submit a string, an explicit null, or no value or at all:

param foo string?

For the consumer of a template, a nullable param is indistinguishable from a regular param with a default value. For the example of foo above, the experience of deploying the containing template is the same as if it had been declared with a default value (param foo string = ''). In both cases, the foo param is optional, and a string, a null, or no value at all will be accepted at deployment time.

Template authors should use a nullable parameter when they are not responsible for deciding what to do in case a param is omitted. A good use case is when a module has an optional param and it is the module's responsibility to assign a default. For example, in the following set of templates, main.bicep serves as an orchestrator, mod.bicep deploys a subsystem, and storage.bicep is a utility module that deploys a storage account:

main.bicep

param modStorageAccountName string?

module mod './mod.bicep' = {
  name: 'mod'
  params: {
    storageAccountName: modStorageAccountName
  }
}

mod.bicep

param storageAccountName string = 'modsa'

module storage './storage.bicep' = {
  name: 'storage'
  params: {
    accountName: storageAccountName
  }
}

storage.bicep

param accountName string

resource sa 'Microsoft.Storage/storageAccounts@2021-08-01' = {
  name: accountName
  ...
}

Because nullable params are meant to provide a means of making a param optional without needing to supply a default value, they are not compatible with default values.

Nullable type expressions may also be used in type statements and in aggregate user-defined types. To avoid creating an artificial distinction between null values and omitted values, nullable types replace optional properties. Once this PR is merged, the following will no longer be valid Bicep:

type myObject = {
  property?: string
}

and would need to instead be expressed as

type myObject = {
  property: string?
}

This PR requires an update to the ARM runtime that has not finished rolling out (included in w02 release). The rollout has completed; this PR is no longer blocked.

Microsoft Reviewers: Open in CodeFlow

@jeskew jeskew force-pushed the jeskew/nullable-types branch 6 times, most recently from 5a49560 to cd10b2c Compare January 9, 2023 22:07
@@ -42,6 +44,8 @@ public Decorator(FunctionOverload overload, TypeSymbol attachableType, Decorator

public void Validate(DecoratorSyntax decoratorSyntax, TypeSymbol targetType, ITypeManager typeManager, IBinder binder, IDiagnosticWriter diagnosticWriter)
{
targetType = RemoveImplicitNull(targetType);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is the right approach. All existing ARM constraints (except type) are no-ops if invoked on a value of null, but extensibility providers may have a need to block usage of an unforeseen decorator on nullable params or types.

The alternative is to update all the decorator definitions with assignability restrictions in SystemNamespaceType so that they accept nullable types (e.g., @minValue would need to be assignable to int | null instead of just int). This might make us more likely to accidentally block a future decorator's usage on nullable params, though, and is more verbose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little quirky, but in the interest of not changing existing behavior, and with the fact that decorators are very infrequently added, I think it's OK - as long as you explain this well in a comment to avoid future surprises.

@jeskew jeskew marked this pull request as ready for review January 12, 2023 01:06
# Conflicts:
#	src/Bicep.Core.IntegrationTests/UserDefinedTypeTests.cs
#	src/Bicep.Core.Samples/Files/TypeDeclarations_LF/main.bicep
#	src/Bicep.Core.Samples/Files/TypeDeclarations_LF/main.diagnostics.bicep
#	src/Bicep.Core.Samples/Files/TypeDeclarations_LF/main.formatted.bicep
#	src/Bicep.Core.Samples/Files/TypeDeclarations_LF/main.json
#	src/Bicep.Core.Samples/Files/TypeDeclarations_LF/main.sourcemap.bicep
#	src/Bicep.Core.Samples/Files/TypeDeclarations_LF/main.symbolicnames.json
#	src/Bicep.Core.Samples/Files/TypeDeclarations_LF/main.symbols.bicep
#	src/Bicep.Core.Samples/Files/TypeDeclarations_LF/main.syntax.bicep
#	src/Bicep.Core.Samples/Files/TypeDeclarations_LF/main.tokens.bicep
#	src/Bicep.Core/Diagnostics/DiagnosticBuilder.cs
#	src/Bicep.Core/Parsing/BaseParser.cs
#	src/Bicep.Core/TypeSystem/ObjectTypeNameBuilder.cs
# Conflicts:
#	src/Bicep.Core.Samples/Files/LoopsIndexed_LF/main.symbolicnames.json
#	src/Bicep.Core.Samples/Files/Loops_LF/main.symbolicnames.json
#	src/Bicep.Core.Samples/Files/ResourcesTenant_CRLF/main.symbolicnames.json
#	src/Bicep.Core.Samples/Files/user_submitted/201/wvd-create-hostpool/main.symbolicnames.json
#	src/Bicep.Core.Samples/Files/user_submitted/301/sql-database-with-management/main.symbolicnames.json
#	src/Bicep.Core.Samples/Files/user_submitted/301/sql-database-with-management/modules/sql-logical-servers.symbolicnames.json
jeskew added a commit that referenced this pull request Jan 23, 2023
jeskew added a commit that referenced this pull request Jan 23, 2023
…anticModel (#9621)

* Spin off a small refactor from #9454

* Update src/Bicep.Core/Diagnostics/DiagnosticBuilder.cs

Co-authored-by: Anthony Martin <38542602+anthony-c-martin@users.noreply.github.com>

* Incorporate PR feedback

Co-authored-by: Anthony Martin <38542602+anthony-c-martin@users.noreply.github.com>
# Conflicts:
#	src/Bicep.Cli.IntegrationTests/packages.lock.json
#	src/Bicep.Cli.UnitTests/packages.lock.json
#	src/Bicep.Cli/packages.lock.json
#	src/Bicep.Core.IntegrationTests/packages.lock.json
#	src/Bicep.Core.Samples/packages.lock.json
#	src/Bicep.Core.UnitTests/Semantics/ArmTemplateSemanticModelTests.cs
#	src/Bicep.Core.UnitTests/packages.lock.json
#	src/Bicep.Core/Diagnostics/DiagnosticBuilder.cs
#	src/Bicep.Core/Semantics/ArmTemplateSemanticModel.cs
#	src/Bicep.Decompiler.IntegrationTests/packages.lock.json
#	src/Bicep.Decompiler.UnitTests/packages.lock.json
#	src/Bicep.Decompiler/packages.lock.json
#	src/Bicep.LangServer.IntegrationTests/packages.lock.json
#	src/Bicep.LangServer.UnitTests/packages.lock.json
#	src/Bicep.LangServer/packages.lock.json
#	src/Bicep.RegistryModuleTool.IntegrationTests/packages.lock.json
#	src/Bicep.RegistryModuleTool.TestFixtures/packages.lock.json
#	src/Bicep.RegistryModuleTool.UnitTests/packages.lock.json
#	src/Bicep.RegistryModuleTool/packages.lock.json
#	src/Bicep.Tools.Benchmark/packages.lock.json
#	src/Bicep.Wasm/packages.lock.json
# Conflicts:
#	src/Bicep.Core.IntegrationTests/UserDefinedTypeTests.cs
#	src/Bicep.Core.Samples/Files/TypeDeclarations_LF/main.json
#	src/Bicep.Core.Samples/Files/TypeDeclarations_LF/main.symbolicnames.json
#	src/Bicep.Core.Samples/Files/Variables_LF/main.symbolicnames.json
#	src/Bicep.Core/Diagnostics/DiagnosticBuilder.cs
#	src/Bicep.Core/Parsing/BaseParser.cs
#	src/Bicep.LangServer/Completions/BicepCompletionProvider.cs
@@ -149,15 +155,47 @@ public override void VisitFunctionCallSyntax(FunctionCallSyntax syntax)
base.VisitFunctionCallSyntax(syntax);
}

public override void VisitObjectTypePropertySyntax(ObjectTypePropertySyntax syntax)
public override void VisitNullableTypeSyntax(NullableTypeSyntax syntax)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, might be worth exploring an approach which utilizes the visitor state itself rather than an ad-hoc check using the syntax hierarchy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find a reasonable way to implement this check in CyclicCheckVisitor without reimplementing some of the model's type manager, but the check can be made much simpler if it's performed after type checking has been performed. I added a second cyclic checker that runs as part of EmitLimitationCalculator.

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

Successfully merging this pull request may close these issues.

?? operator eval check for empty string if type is string Empty default parameter value passed to modules
2 participants