Decorators #1324

shenglol · 2021-01-15T08:00:54Z

Added support to decorators. There are some features that I'm planning on adding in follow-up PRs:

Code intelligence
Decompiling

The other thing worth mentioning is that the current parameter modifier syntax will continue working. My idea is to give users sometime to migrate to the new syntax. We can drop modifier support before the 0.3 release if there's no major concerns. I've added semantic checking to ban using decorators and modifier at the same time. Once the PR is merged, I'll be working on updating the examples and test files to use decorators (this can take a while and I would expect some helps from the OSS community). Once they are all updated, I'll implement another checker to warn users that parameter modifier is deprecated.

Partially addresses #64.

src/Bicep.Core/Parsing/Parser.cs

codecov-io · 2021-01-19T23:35:23Z

Codecov Report

Merging #1324 (70907f4) into main (7207ddf) will decrease coverage by 0.05%.
The diff coverage is 93.22%.

@@            Coverage Diff             @@
##             main    #1324      +/-   ##
==========================================
- Coverage   94.81%   94.76%   -0.06%     
==========================================
  Files         343      351       +8     
  Lines       17564    18188     +624     
  Branches       14       14              
==========================================
+ Hits        16653    17235     +582     
- Misses        911      953      +42

Flag	Coverage Δ
dotnet	`95.26% <93.22%> (-0.08%)`	⬇️
typescript	`27.20% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
....Core.UnitTests/Utils/SyntaxTreeGroupingFactory.cs	`100.00% <ø> (ø)`
src/Bicep.Core/Syntax/SyntaxRewriteVisitor.cs	`71.31% <46.15%> (-2.65%)`	⬇️
src/Bicep.Core/Syntax/SyntaxFactory.cs	`58.13% <58.13%> (ø)`
src/Bicep.Core/Semantics/DecoratorBuilder.cs	`77.77% <77.77%> (ø)`
src/Bicep.Core/TypeSystem/CyclicCheckVisitor.cs	`95.23% <88.88%> (+1.12%)`	⬆️
src/Bicep.Core/TypeSystem/TypeAssignmentVisitor.cs	`96.33% <90.00%> (-0.46%)`	⬇️
src/Bicep.Core/Syntax/ObjectSyntaxExtensions.cs	`93.93% <90.47%> (-6.07%)`	⬇️
src/Bicep.Core/Semantics/Decorator.cs	`92.30% <92.30%> (ø)`
src/Bicep.Core/TypeSystem/DecoratorResolver.cs	`92.85% <92.85%> (ø)`
src/Bicep.Core/Semantics/SymbolValidator.cs	`83.33% <96.15%> (+6.25%)`	⬆️
... and 47 more

src/Bicep.Core/TypeSystem/TypeAssignmentVisitor.cs

shenglol · 2021-01-22T01:13:38Z

src/Bicep.Core/Syntax/SyntaxFactory.cs

+
+namespace Bicep.Core.Syntax
+{
+    public static class SyntaxFactory


We now have multiple similar classes defined in different projects:

Bicep.Core.Syntax.SyntaxFactory

Bicep.Decompiler.BicepHelpers.SyntaxHelpers

Bicep.Core.UnitTests.Utils.TestSyntaxFactory

To make this PR less overwhelming, I'll send a separate PR to reduce them to one class.

anthony-c-martin · 2021-01-25T14:52:20Z

src/Bicep.Core/Parsing/Parser.cs

+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+using System;
+using System.Collections.Generic;
+using System.Collections.Immutable;
+using System.Globalization;
+using System.Linq;
+using Bicep.Core.Diagnostics;
+using Bicep.Core.Extensions;
+using Bicep.Core.Navigation;
+using Bicep.Core.Syntax;
+
+namespace Bicep.Core.Parsing
+{
+    public class Parser
+    {
+        private readonly TokenReader reader;
+


@shenglol - do you know why your PRs show large whitespace diffs? Are there line ending changes here? If so, any way to avoid this? I notice that when I modify code afterwards, I also get large whitespace diffs (presumably to change back to the previous format). It would be good if we can converge on / enforce a standard here.

I suspect that the line endings of this file got normalized by someone's Visual Studio settings. The whitespace diffs showed up after I merged the main branch. I feel like we should put a .editorconfig file at the root level and add that as a VS solution item. I submit a PR to do that later.

Created #1401 to track this.

src/Bicep.Core/Parsing/Parser.cs

anthony-c-martin · 2021-01-25T14:58:00Z

src/Bicep.Core/Parsing/Parser.cs

+                            LanguageConstants.TargetScopeKeyword => this.TargetScope(leadingNodes),
+                            LanguageConstants.ParameterKeyword => this.ParameterDeclaration(leadingNodes),
+                            LanguageConstants.VariableKeyword => this.VariableDeclaration(leadingNodes),
+                            LanguageConstants.ResourceKeyword => this.ResourceDeclaration(leadingNodes),
+                            LanguageConstants.OutputKeyword => this.OutputDeclaration(leadingNodes),
+                            LanguageConstants.ModuleKeyword => this.ModuleDeclaration(leadingNodes),


Out of interest, what are the pros/cons of modelling decorators as part of the declaration syntax, vs as a top-level syntax independent of the declarations? Do you happen to know how this is handled in similar languages (C# or TS for example)?

Roslyn includes under the declaration:

For the missing declaration case, they create an IncompleteMember node:

I suspect having the decorators under the declaration makes it easier to process them. Otherwise, you would always need a visitor to collect the preceding decorators. (Or the ability to efficiently get the preceding peer node in the AST.)

TypeScript also puts them under declarations probably for the reason that Marcin mentioned:

src/Bicep.Core/Parsing/Parser.cs

src/Bicep.Core/Semantics/Namespaces/AzNamespaceSymbol.cs

majastrz · 2021-01-25T21:56:52Z

src/Bicep.Core/Semantics/Decorator.cs

+        TypeSymbol targetType,
+        ObjectSyntax? targetObject);
+
+    public class Decorator


Decorator [](start = 17, length = 9)

Is there any value in making decorators symbols? It might be helpful with some of the navigation features like hovers to avoid special casing...

I thought about creating a new DecoratorSymbol type originally, but on second thought modelling them as FunctionSymbols makes it easier to leverage all semantic checkers and LSP capablilities we have implemented for function calls (decorators are essentially higher order functions IMHO). Features like hovering is already working:

Do you mind clarifying what "to avoid special casing" means?

Can ignore that comment if hovers are already working. I think the hover text will be confusing to our users. We should probably make it more obvious that it's a decorator. (Higher order functions are an advanced concept after all.)

Agreed that we should make it more obvious that it's a decorator. Maybe showing descriptions below type signatures would help, but I feel like that's something to be implemented in another PR and we probably need to discuss it.

src/Bicep.Core.Samples/Files/InvalidParameters_LF/main.diagnostics.bicep

src/Bicep.Core/Semantics/Namespaces/SystemNamespaceSymbol.cs

majastrz · 2021-01-27T23:38:46Z

src/Bicep.Core/Syntax/DeclarationSyntax.cs

+
+namespace Bicep.Core.Syntax
+{
+    public abstract class StatementSyntax : SyntaxBase


StatementSyntax [](start = 26, length = 15)

Should we call it DeclarationSyntax to match the file name?

Ah good catch...too bad my ReSharper license expired which should have caught this. I renamed the file to StatementSyntax though. It's kind of weird if we call it DeclarationSyntax while it has nothing to do with the IDeclaration interface. Let me know if StatementSyntax works.

majastrz · 2021-01-27T23:39:59Z

src/Bicep.Core/Syntax/DeclarationSyntax.cs

+            this.LeadingNodes = leadingNodes.ToImmutableArray();
+        }
+
+        public ImmutableArray<SyntaxBase> LeadingNodes { get; }


LeadingNodes [](start = 42, length = 12)

I like how adding this class cleaned up all the declarations btw!

src/Bicep.Core.Samples/Files/InvalidExpressions_LF/main.bicep

majastrz · 2021-01-27T23:52:35Z

At some point (not necessarily in this PR), we should update the parameter snippets and switch them to the decorator syntax.

majastrz · 2021-01-27T23:58:46Z

It looks like you have to type @secure() and it seems @secure is not enough. Is that intentional and should we relax that in the future?

majastrz · 2021-01-28T00:00:28Z

I tried doing a dangling decorator:

Can we make it more obvious that the decorator requires a declaration to follow? (The error message is in the right position, so that definitely helps to figure what is going on.)

majastrz · 2021-01-28T00:02:25Z

Doing @secure on an int parameter didn't give me any errors btw (tried in VS code and CLI):

I think we have a test case for this with the modifier syntax. Should probably copy and convert all those tests to decorators so we preserve coverage.

shenglol · 2021-01-28T08:41:04Z

I tried doing a dangling decorator:

Can we make it more obvious that the decorator requires a declaration to follow? (The error message is in the right position, so that definitely helps to figure what is going on.)

Makes sense. The current one is a bit confusing when there's no declaration under decorators. I tweaked the parser to achieve this:

shenglol · 2021-01-28T08:42:46Z

Doing @secure on an int parameter didn't give me any errors btw (tried in VS code and CLI):

I think we have a test case for this with the modifier syntax. Should probably copy and convert all those tests to decorators so we preserve coverage.

Yeah we should. I copied and converted all parameter tests and it did help capture some edge cases that I missed. I'll keep updating more as I start converting other test files (PRs to come).

shenglol · 2021-01-28T18:49:51Z

It looks like you have to type @secure() and it seems @secure is not enough. Is that intentional and should we relax that in the future?

Had a discussion offline with Marcin. The reason why I didn't implement @secure in this PR is that we should also support @sys.secure, but currently name binding doesn't work for property access syntax. We'll talk about this during our next DSL discussion meeting.

alex-frankel · 2021-01-28T18:51:28Z

The reason why I didn't implement @secure in this PR is that we should also want to support @sys.secure, but currently naming binding doesn't work for property access syntax. We'll talk about this during our next DSL discussion meeting.

Can we create an issue to track this?

shenglol · 2021-01-28T19:02:26Z

The reason why I didn't implement @secure in this PR is that we should also want to support @sys.secure, but currently naming binding doesn't work for property access syntax. We'll talk about this during our next DSL discussion meeting.

Can we create an issue to track this?

Yep. Created an issue: #1405.

majastrz · 2021-01-28T19:25:08Z

src/Bicep.Core/Parsing/Parser.cs

+                    // If there are dangling decorators and we hit EOF, ask users to add a declration.
+                    // Set the span of the diagnostic so that it's on the line below the last decorator.
+                    var lastNewLine = missingDeclaration.LeadingNodes.Last(node => node is Token { Type: TokenType.NewLine });
+                    var diagnosticSpan = new TextSpan(lastNewLine.Span.Position + 2, 0);


The + 2 approach only works on CRLF files. On LF, the position is off by 1:

If you don't have a newline following the decorator, the parser will tell you to add it, so it seems like you can rely on it and use its position, but not sure if there are other edge cases.

I'm fine if this goes into the next PR also.

In reply to: 566350593 [](ancestors = 566350593)

This is to handle an edge case where there are multiple newlines after a decorator:

Not sure why the error span is [3, 2]. On my machine it's [3, 1]...

It's due to different line endings.

majastrz

Found an off by 1 bug with LF files (added a comment), but otherwise looks great!

shenglol · 2021-01-28T19:38:35Z

Found an off by 1 bug with LF files (added a comment), but otherwise looks great!

Will send a separate PR to fix that soon.

TSunny007 reviewed Jan 15, 2021

View reviewed changes

src/Bicep.Core/Parsing/Parser.cs Show resolved Hide resolved

Parsing and semantic analysis for decorators

f5a9a32

shenglol force-pushed the shenglol/decorators branch from 07a5675 to f5a9a32 Compare January 19, 2021 23:21

shenglol added 10 commits January 19, 2021 20:40

First iteration of decorator emitter

0856841

Simplify decorator definitions

e2b712a

Remove constructors not needed

785b75b

Emit default value

0632101

Ban using parameter decorators and modifiers together

363b323

Minor refactoring

16aafc9

Add formatting support for decorators

dfbac3b

Fix parser and spans

4ac9156

Improve ObjectSyntax merging methods

16b9e94

Update test baselines

133f8ec

shenglol marked this pull request as ready for review January 22, 2021 00:42

shenglol changed the title ~~[WIP] Decorators~~ Decorators Jan 22, 2021

shenglol commented Jan 22, 2021

View reviewed changes

src/Bicep.Core/TypeSystem/TypeAssignmentVisitor.cs Show resolved Hide resolved

shenglol commented Jan 22, 2021

View reviewed changes

shenglol added 2 commits January 22, 2021 11:14

Add missing test cases

ac2e9aa

Merge branch 'main' into shenglol/decorators

46c31b6

shenglol linked an issue Jan 22, 2021 that may be closed by this pull request

Decorator syntax #64

Closed

anthony-c-martin reviewed Jan 25, 2021

View reviewed changes

src/Bicep.Core/Parsing/Parser.cs Show resolved Hide resolved

anthony-c-martin reviewed Jan 25, 2021

View reviewed changes

src/Bicep.Core/Parsing/Parser.cs Outdated Show resolved Hide resolved

anthony-c-martin reviewed Jan 25, 2021

View reviewed changes

src/Bicep.Core/Semantics/Namespaces/AzNamespaceSymbol.cs Outdated Show resolved Hide resolved

anthony-c-martin reviewed Jan 25, 2021

View reviewed changes

src/Bicep.Core/Semantics/Namespaces/AzNamespaceSymbol.cs Outdated Show resolved Hide resolved

Remove unnecessary recovery logic

a2451cb

majastrz reviewed Jan 25, 2021

View reviewed changes

src/Bicep.Core.Samples/Files/InvalidParameters_LF/main.diagnostics.bicep Show resolved Hide resolved

shenglol added 4 commits January 25, 2021 16:47

Move decorators to sys namespace

13cea1c

Move leading nodes to a base class

6b0507b

Handle dangling decorators

cf1697d

Add grammar for decorators

7a2df8a

majastrz reviewed Jan 27, 2021

View reviewed changes

src/Bicep.Core/Semantics/Namespaces/SystemNamespaceSymbol.cs Outdated Show resolved Hide resolved

majastrz reviewed Jan 27, 2021

View reviewed changes

src/Bicep.Core.Samples/Files/InvalidExpressions_LF/main.bicep Outdated Show resolved Hide resolved

Fix a file name; clean up garbage

a1ac8bc

shenglol added 4 commits January 27, 2021 18:13

Add missing validator for @secure()

14963fd

Add a note to warn people to not break dangling decorators tests

8cee64e

Fine-tune decorator parsing logic

6b2e7b2

Copy and convert parameter modifier tests to use decorators

9d24c78

shenglol added 2 commits January 28, 2021 10:24

Merge branch 'main' into shenglol/decorators

7da0ef0

Merge branch 'main' into shenglol/decorators

70907f4

majastrz reviewed Jan 28, 2021

View reviewed changes

majastrz approved these changes Jan 28, 2021

View reviewed changes

shenglol merged commit 3c12faa into main Jan 28, 2021

shenglol deleted the shenglol/decorators branch January 28, 2021 19:38

shenglol mentioned this pull request Jan 28, 2021

Place BCP132 on the last dangling decorator #1407

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decorators #1324

Decorators #1324

shenglol commented Jan 15, 2021 •

edited

Loading

codecov-io commented Jan 19, 2021 •

edited

Loading

shenglol Jan 22, 2021 •

edited

Loading

anthony-c-martin Jan 25, 2021

shenglol Jan 25, 2021

shenglol Jan 28, 2021

anthony-c-martin Jan 25, 2021

majastrz Jan 25, 2021

majastrz Jan 25, 2021

shenglol Jan 25, 2021 •

edited

Loading

majastrz Jan 25, 2021

shenglol Jan 25, 2021

majastrz Jan 25, 2021

shenglol Jan 26, 2021

majastrz Jan 27, 2021

shenglol Jan 28, 2021 •

edited

Loading

majastrz Jan 27, 2021 •

edited

Loading

majastrz commented Jan 27, 2021

majastrz commented Jan 27, 2021

majastrz commented Jan 28, 2021

majastrz commented Jan 28, 2021 •

edited

Loading

shenglol commented Jan 28, 2021 •

edited

Loading

shenglol commented Jan 28, 2021 •

edited

Loading

shenglol commented Jan 28, 2021 •

edited

Loading

alex-frankel commented Jan 28, 2021

shenglol commented Jan 28, 2021

majastrz Jan 28, 2021

majastrz Jan 28, 2021

shenglol Jan 28, 2021

shenglol Jan 28, 2021

majastrz left a comment

shenglol commented Jan 28, 2021

Decorators #1324

Decorators #1324

Conversation

shenglol commented Jan 15, 2021 • edited Loading

codecov-io commented Jan 19, 2021 • edited Loading

Codecov Report

shenglol Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenglol Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenglol Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

majastrz Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

majastrz commented Jan 27, 2021

majastrz commented Jan 27, 2021

majastrz commented Jan 28, 2021

majastrz commented Jan 28, 2021 • edited Loading

shenglol commented Jan 28, 2021 • edited Loading

shenglol commented Jan 28, 2021 • edited Loading

shenglol commented Jan 28, 2021 • edited Loading

alex-frankel commented Jan 28, 2021

shenglol commented Jan 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majastrz left a comment

Choose a reason for hiding this comment

shenglol commented Jan 28, 2021

shenglol commented Jan 15, 2021 •

edited

Loading

codecov-io commented Jan 19, 2021 •

edited

Loading

shenglol Jan 22, 2021 •

edited

Loading

shenglol Jan 25, 2021 •

edited

Loading

shenglol Jan 28, 2021 •

edited

Loading

majastrz Jan 27, 2021 •

edited

Loading

majastrz commented Jan 28, 2021 •

edited

Loading

shenglol commented Jan 28, 2021 •

edited

Loading

shenglol commented Jan 28, 2021 •

edited

Loading

shenglol commented Jan 28, 2021 •

edited

Loading