Repository navigation
Unsafe evolution: unsafe expressions - #84012
Conversation
|
This PR modifies public API files. Please follow the instructions at https://github.com/dotnet/roslyn/blob/main/docs/contributing/API%20Review%20Process.md for ensuring all public APIs are reviewed before merging. |
There was a problem hiding this comment.
Pull request overview
Adds C# parser/binder and public syntax model support for a new primary-function form unsafe(<expr>) (Unsafe Evolution), including updates to syntax generation outputs and extensive parsing/semantic tests.
Changes:
- Introduces
SyntaxKind.UnsafeExpression/UnsafeExpressionSyntax, parsing (LanguageParser.ParseUnsafeExpression) and binding (BindUnsafeExpression+ unsafe-region binder flags). - Updates semantic model/binder mapping utilities to treat
unsafe(<expr>)like other parenthesized constructs for binder association and “bindable node” selection. - Adds parsing tests, await-parsing coverage, and CSharp15 Unsafe Evolution semantic tests (diagnostics, constant value, IOperation, flow graph, IL).
Reviewed changes
Copilot reviewed 13 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Compilers/CSharp/Test/Syntax/Parsing/ExpressionParsingTests.cs | Adds parse-tree coverage for unsafe(<expr>) and common postfix forms. |
| src/Compilers/CSharp/Test/Syntax/Parsing/AwaitParsingTests.cs | Ensures await unsafe(...) parses as AwaitExpression over UnsafeExpression. |
| src/Compilers/CSharp/Test/Syntax/Generated/Syntax.Test.xml.Generated.cs | Adds generated factory/rewriter tests for UnsafeExpressionSyntax. |
| src/Compilers/CSharp/Test/CSharp15/UnsafeEvolutionTests.cs | Adds end-to-end tests for diagnostics and semantics of unsafe(<expr>) under updated memory safety rules. |
| src/Compilers/CSharp/Portable/Syntax/SyntaxNodeExtensions.cs | Allows UnsafeExpression to have an associated local binder. |
| src/Compilers/CSharp/Portable/Syntax/SyntaxKindFacts.cs | Maps UnsafeKeyword as a “primary function” keyword to UnsafeExpression. |
| src/Compilers/CSharp/Portable/Syntax/SyntaxKind.cs | Introduces SyntaxKind.UnsafeExpression (experimental). |
| src/Compilers/CSharp/Portable/Syntax/Syntax.xml | Defines UnsafeExpressionSyntax node in the syntax model generator input. |
| src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt | Tracks new public API surface for UnsafeExpressionSyntax and visitors/factory methods. |
| src/Compilers/CSharp/Portable/Parser/LanguageParser.cs | Parses unsafe(<expr>) and disambiguates statement vs expression uses. |
| src/Compilers/CSharp/Portable/Generated/CSharpSyntaxGenerator/CSharpSyntaxGenerator.SourceGenerator/Syntax.xml.Syntax.Generated.cs | Generated red node for UnsafeExpressionSyntax. |
| src/Compilers/CSharp/Portable/Generated/CSharpSyntaxGenerator/CSharpSyntaxGenerator.SourceGenerator/Syntax.xml.Main.Generated.cs | Generated visitors/rewriter + SyntaxFactory.UnsafeExpression API. |
| src/Compilers/CSharp/Portable/Generated/CSharpSyntaxGenerator/CSharpSyntaxGenerator.SourceGenerator/Syntax.xml.Internal.Generated.cs | Generated green node + internal factory support for UnsafeExpression. |
| src/Compilers/CSharp/Portable/Generated/CSharp.Generated.g4 | Updates grammar to include unsafe_expression in expressions. |
| src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs | Treats UnsafeExpressionSyntax similarly to other parenthesized constructs for binder lookup and binding selection. |
| src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs | Creates an unsafe-region binder for UnsafeExpressionSyntax. |
| src/Compilers/CSharp/Portable/Binder/EarlyWellKnownAttributeBinder.cs | Allows UnsafeExpression as a syntactically valid attribute-argument form (rebinding later produces diagnostics as needed). |
| src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs | Binds UnsafeExpressionSyntax and enforces /unsafe + feature availability. |
|
@333fred @AlekseyTs for reviews, thanks |
|
Does this PR introduce a breaking change? |
No, I don't think so. |
| { | ||
| return _syntaxFactory.GlobalStatement(ParseUnsafeStatement(attributes)); | ||
| } | ||
| else if (this.PeekToken(1).Kind == SyntaxKind.OpenParenToken) |
There was a problem hiding this comment.
I guess the following comment below explains that
// All modifiers that might start an expression are processed above.
| } | ||
| else | ||
| { | ||
| CheckFeatureAvailability(node.UnsafeKeyword, MessageID.IDS_FeatureUnsafeEvolution, diagnostics); |
There was a problem hiding this comment.
This was done intentionally to match what Binder.BindUnsafeStatement does. But I agree I don't see much advantage in this.
| return ParseExpressionStatement(attributes); | ||
| } | ||
|
|
||
| return IsPossibleUnsafeStatement() ? ParseUnsafeStatement(attributes) : null; |
| // checked and unchecked expressions | ||
| case SyntaxKind.UncheckedExpression: | ||
| case SyntaxKind.CheckedExpression: | ||
| case SyntaxKind.UnsafeExpression: |
There was a problem hiding this comment.
Adding UnsafeEvolutionTests.UnsafeExpression_AttributeArgument_DefaultParameterValue, thanks.
| var expectedOperationTree = """ | ||
| IReturnOperation (OperationKind.Return, Type: null) (Syntax: 'return unsafe(*p);') | ||
| ReturnedValue: | ||
| IOperation: (OperationKind.None, Type: System.Int32) (Syntax: '*p') |
There was a problem hiding this comment.
IOperation: (OperationKind.None, Type: System.Int32) (Syntax: '*p')
Apparently we haven't implemented support for dereference (and likely other pointer specific operations) For IOperation tests, consider using supported forms of expressions. Also, given the feature, should we add support for the pointer related operations?
There was a problem hiding this comment.
Also, given the feature, should we add support for the pointer related operations?
The feature only makes pointer types safe, pointer dereference is still unsafe as it always was, i.e., adding IOperation for it doesn't seem particularly motivated by this feature.
There was a problem hiding this comment.
The feature only makes pointer types safe, pointer dereference is still unsafe as it always was, i.e., adding IOperation for it doesn't seem particularly motivated by this feature.
Pointers and operations on pointers were "outside" of the language. It is my understanding, that with this feature, they truly become part of the language.
There was a problem hiding this comment.
To clarify, I am not suggesting to do the IOperation work in this PR
There was a problem hiding this comment.
Pointers and operations on pointers were "outside" of the language. It is my understanding, that with this feature, they truly become part of the language.
That's a good point, filed #84081.
| Predecessors: [B0] | ||
| Statements (0) | ||
| Next (Return) Block[B2] | ||
| IOperation: (OperationKind.None, Type: System.Int32) (Syntax: '*p') |
| CreateCompilation(source, | ||
| options: TestOptions.UnsafeReleaseExe.WithUpdatedMemorySafetyRules()) | ||
| .VerifyDiagnostics( | ||
| // (2,1): error CS0201: Only assignment, call, increment, decrement, await, and new object expressions can be used as a statement |
| { | ||
| if (LookupPosition.IsBetweenTokens(position, unsafeExpression.OpenParenToken, unsafeExpression.CloseParenToken)) | ||
| { | ||
| binder = rootBinder.GetBinder(current); |
There was a problem hiding this comment.
Adding UnsafeExpression_SemanticModel_EnclosingBinder, thanks.
|
Done with review pass (commit 7) |
|
Done with review pass (commit 10). It looks like there are legitimate CI failures |
|
@333fred for a second review, thanks |
| </Field> | ||
| <Field Name="Expression" Type="ExpressionSyntax"> | ||
| <PropertyComment> | ||
| <summary>Argument of the primary function.</summary> |
There was a problem hiding this comment.
This comment doesn't make sense to me?
There was a problem hiding this comment.
Yeah, it was copied from the CheckedExpressionSyntax, but it doesn't make sense to me either. I will improve it here.
There was a problem hiding this comment.
So copilot found a fun edge case:
void M()
{
unsafe (int, int) Local() => (1, 2); // valid before this PR, broken after.
}There was a problem hiding this comment.
Let's also test in a catch filter block, as they can do interesting things sometimes.
There was a problem hiding this comment.
Let's also test in a catch filter block, as they can do interesting things sometimes.
We are already testing that in UnsafeEvolutionTests.UnsafeExpression_ExceptionFilter.
| => PeekToken(1).Kind == SyntaxKind.OpenParenToken ? ParseUsingStatement(attributes) : ParseLocalDeclarationStatement(attributes); | ||
|
|
||
| // Checking for brace to disambiguate between unsafe statement and unsafe local function | ||
| // Checking for brace or parenthesis to disambiguate between unsafe statement, unsafe expression, and unsafe local function |
There was a problem hiding this comment.
Plural
| // Checking for brace or parenthesis to disambiguate between unsafe statement, unsafe expression, and unsafe local function | |
| // Checking for brace or parentheses to disambiguate between unsafe statement, unsafe expression, and unsafe local function |
| { | ||
| int M(int i) | ||
| /*<bind>*/{ | ||
| return unsafe(i); |
There was a problem hiding this comment.
We should have a version of this test with flow inside the unsafe, like a ternary or null-coalscing expression.
Test plan: #81207
Speclet update: dotnet/csharplang#10196
Microsoft Reviewers: Open in CodeFlow