-
Notifications
You must be signed in to change notification settings - Fork 816
Quick Fixes to create new parameters or variables when there are undefined symbols in bicep files. #18654
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
base: main
Are you sure you want to change the base?
Conversation
|
@microsoft-github-policy-service agree |
| /// </summary> | ||
| public class UndefinedSymbolCodeFixProvider : ICodeFixProvider | ||
| { | ||
| private const string DiagnosticCode = "BCP057"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than retrospectively running this analysis on diagnostic codes, the approach we'd like to take is to use the Fixes array when raising the diagnostics in he first place. This doesn't require you to write a code fix provider.
See here for an example of what this looks like:
bicep/src/Bicep.Core/Diagnostics/DiagnosticBuilder.cs
Lines 1264 to 1268 in 63b642b
| public IDiagnostic MissingParameterAssignment(IEnumerable<string> identifiers, CodeFix insertMissingCodefix) => CoreError( | |
| "BCP258", | |
| $"The following parameters are declared in the Bicep file but are missing an assignment in the params file: {ToQuotedString(identifiers)}.") | |
| with | |
| { Fixes = [insertMissingCodefix] }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! I'm in the process of implementing the requested changes.
However, this one seems difficult in practice without sacrificing a lot of the functionality. If we want to keep the type inferring logic, formatting and placing the declarations in context etc. we do require SemanticModel, TypeManager, PrettyPrinterV2Context and so on. Those are not available when the diagnostic is raised in the first place, unless I'm totally mistaken.
Currently my refactored approach would move the implementation to Bicep.Core from the LangServer, and handle the creation in SemanticModel.AssembleDiagnostics() with
var disabledDiagnosticsCache = SourceFile.DisabledDiagnosticsCache;
foreach (IDiagnostic diagnostic in diagnostics)
{
(int diagnosticLine, _) = TextCoordinateConverter.GetPosition(SourceFile.LineStarts, diagnostic.Span.Position);
if (diagnosticLine == 0 || !diagnostic.CanBeSuppressed())
{
filteredDiagnostics.Add(AugmentDiagnostic(diagnostic)); // Here
continue;
}
if (disabledDiagnosticsCache.TryGetDisabledNextLineDirective(diagnosticLine - 1) is { } disableNextLineDirectiveEndPositionAndCodes &&
disableNextLineDirectiveEndPositionAndCodes.diagnosticCodes.Contains(diagnostic.Code))
{
continue;
}
filteredDiagnostics.Add(AugmentDiagnostic(diagnostic)); // And here
}
// ...snipped...
/// <summary>
/// Applies any diagnostic enrichments that require full semantic context
/// (for example, attaching code fixes) before diagnostics are surfaced
/// outside of the core compiler.
/// </summary>
private IDiagnostic AugmentDiagnostic(IDiagnostic diagnostic) => diagnostic.Code switch
{
// BCP057 (undefined symbol) quick fixes depend on semantic information
// such as type inference and declaration insertion points, which are
// only available once the semantic model has been fully constructed.
"BCP057" => AugmentWithUndefinedSymbolFixes(diagnostic),
_ => diagnostic,
};Thoughts?
| // Type resolution priority for generated parameters: | ||
| // 1. Module parameter types (preserve exact type from referenced module) | ||
| // 2. Clear usage context (bool in conditions, int in arithmetic) | ||
| // 3. Resource-derived types (e.g., resourceInput<'Microsoft.Storage/storageAccounts@2023-01-01'>.sku) | ||
| // 4. User-defined type aliases (preserve named types when possible) | ||
| // 5. Fallback to TypeStringifier with medium strictness |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeskew - do you know if we have any existing code that could help with generating type syntax?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a TypeStringifier utility that could be used for this.
|
|
||
| var variableInsertionOffset = FindInsertionOffset(parentStatement, typeof(VariableDeclarationSyntax)); | ||
| var defaultInitializer = GetDefaultInitializer(effectiveType); | ||
| var variableText = BuildDeclarationText(variableInsertionOffset, $"var {name} = {defaultInitializer}", newline); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than building syntax with strings, could you use the syntax factory here to generate the syntax tree? This makes it more straightforward to account for edge cases in code generation.
See here for an example:
bicep/src/Bicep.Core/Syntax/SyntaxFactory.cs
Lines 49 to 53 in 63b642b
| public static VariableBlockSyntax CreateVariableBlock(IEnumerable<IdentifierSyntax> variables) | |
| => new( | |
| LeftParenToken, | |
| Interleave(variables.Select(x => new LocalVariableSyntax(x)), () => CommaToken), | |
| SyntaxFactory.RightParenToken); |
| /// <summary> | ||
| /// Checks if the child syntax node is contained within the parent syntax node's span. | ||
| /// This is used instead of reference equality when walking up the syntax tree, | ||
| /// because the child may be wrapped in intermediate nodes (e.g., parentheses). | ||
| /// </summary> | ||
| private static bool IsContainedIn(SyntaxBase child, SyntaxBase parent) => | ||
| child.Span.Position >= parent.Span.Position && | ||
| child.Span.GetEndPosition() <= parent.Span.GetEndPosition(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have existing code to do this:
| public static bool IsOverlapping(this IPositionable positionable, int position) |
| /// Checks (in order): comparison with literals, boolean context, arithmetic, | ||
| /// string interpolation, for-loop iteration, and parent property type. | ||
| /// </summary> | ||
| private static TypeSymbol? InferByContext(SemanticModel semanticModel, VariableAccessSyntax variableAccess) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to use DeclaredTypeManager.GetDeclaredType() to obtain the expected type here, instead of having to write custom logic?
| return $"{declaration}{spacing}"; | ||
| } | ||
|
|
||
| private int CountConsecutiveNewlinesAtOffset(int insertionOffset, string newline) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be possible to reuse/share logic from
| private static CodeReplacement GenerateCodeReplacement(BicepCompiler compiler, RootConfiguration configuration, ResourceDeclarationSyntax resourceDeclaration, InsertContext insertContext) |
anthony-c-martin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
In general, I think there's quite a few existing concepts you should be able to re-use, rather than re-inventing. Definitely appreciate that it is not particularly easy to discover, so I'm very happy to share more detailed code pointers if my comments aren't clear enough, or you're not sure how to proceed.
Description
Adds a quick fix for BCP057 ("The name does not exist in the current context") that offers to create a parameter or variable declaration for undefined symbols. This makes using snippets without included parameters / variables much easier. This was requested in and fixes #12536
When determining the type for a new parameter, the fix first checks usage context (e.g., bool if used in a condition, int if used in arithmetic), then tries resource-derived input types for complex properties, then user-defined type aliases, and finally falls back to TypeStringifier for the inferred type.
Variables try to assign some reasonable defaults when created, inferred from types. However, for objects if there are more than 5 properties, the variable defaults to an empty object to not make too many assumptions which the user might need.
New declarations are inserted after the last existing declaration of the same kind, or at the start of the referencing statement.
Type inference requires semantic model APIs (GetDeclaredType, GetDeclaredTypeAssignment, contextual analysis) that aren't available during diagnostic emission, so this was implemented as an LSP code action rather than attached directly to the diagnostic.
Example Usage
Please check the included tests for further use cases. Here's a video showing some in the IDE.
UndefinedTypesQuickFix.webm
NOTE: Parameters passed through module context pass the resourceInput values through, but defined user types are not explicitly imported.
For example a module:
Calling file's codefix produces
Checklist
Microsoft Reviewers: Open in CodeFlow