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

Code changes to allow 'using static unsafe' #67344

Merged
merged 23 commits into from Mar 21, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/Compilers/CSharp/Portable/CSharpResources.resx
Expand Up @@ -6854,7 +6854,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<value>Using alias cannot be a 'ref' type.</value>
</data>
<data name="ERR_BadUnsafeInUsingDirective" xml:space="preserve">
<value>Only a using alias can be 'unsafe'.</value>
<value>Only a 'using static' or 'using alias' can be 'unsafe'.</value>
</data>
<data name="ERR_BadNullableReferenceTypeInUsingAlias" xml:space="preserve">
<value>Using alias cannot be a nullable reference type.</value>
Expand Down Expand Up @@ -7499,4 +7499,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="WRN_AddressOfInAsync_Title" xml:space="preserve">
<value>The '&amp;' operator should not be used on parameters or local variables in async methods.</value>
</data>
<data name="ERR_BadStaticAfterUnsafe" xml:space="preserve">
<value>'static' modifier must precede 'unsafe' modifier.</value>
</data>
</root>
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Expand Up @@ -2186,6 +2186,7 @@ internal enum ErrorCode
ERR_BadRefInUsingAlias = 9130,
ERR_BadUnsafeInUsingDirective = 9131,
ERR_BadNullableReferenceTypeInUsingAlias = 9132,
ERR_BadStaticAfterUnsafe = 9133,
#endregion

// Note: you will need to do the following after adding warnings:
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
Expand Up @@ -2307,6 +2307,7 @@ internal static bool IsBuildOnlyDiagnostic(ErrorCode code)
case ErrorCode.ERR_BadRefInUsingAlias:
case ErrorCode.ERR_BadUnsafeInUsingDirective:
case ErrorCode.ERR_BadNullableReferenceTypeInUsingAlias:
case ErrorCode.ERR_BadStaticAfterUnsafe:
return false;
default:
// NOTE: All error codes must be explicitly handled in this switch statement
Expand Down
8 changes: 8 additions & 0 deletions src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
Expand Up @@ -796,6 +796,14 @@ private UsingDirectiveSyntax ParseUsingDirective()
var staticToken = this.TryEatToken(SyntaxKind.StaticKeyword);
var unsafeToken = this.TryEatToken(SyntaxKind.UnsafeKeyword);

// if the user wrote `using unsafe static` skip the `static` and tell them it needs to be `using static unsafe`.
if (staticToken is null && unsafeToken != null && this.CurrentToken.Kind == SyntaxKind.StaticKeyword)
{
// create a missing 'static' token so that later binding does recognize what the user wanted.
staticToken = SyntaxFactory.MissingToken(SyntaxKind.StaticKeyword);
unsafeToken = AddTrailingSkippedSyntax(unsafeToken, AddError(this.EatToken(), ErrorCode.ERR_BadStaticAfterUnsafe));
}

var alias = this.IsNamedAssignment() ? ParseNameEquals() : null;

TypeSyntax type;
Expand Down
9 changes: 6 additions & 3 deletions src/Compilers/CSharp/Portable/Symbols/AliasSymbol.cs
Expand Up @@ -377,20 +377,23 @@ private NamespaceSymbol ResolveExternAliasTarget(BindingDiagnosticBag diagnostic
BindingDiagnosticBag diagnostics,
ConsList<TypeSymbol>? basesBeingResolved)
{
var reportedError = false;
if (usingDirective.UnsafeKeyword != default)
{
MessageID.IDS_FeatureUsingTypeAlias.CheckFeatureAvailability(diagnostics, usingDirective, usingDirective.UnsafeKeyword.GetLocation());
reportedError = !MessageID.IDS_FeatureUsingTypeAlias.CheckFeatureAvailability(diagnostics, usingDirective, usingDirective.UnsafeKeyword.GetLocation());
}
else if (usingDirective.NamespaceOrType is not NameSyntax)
{
MessageID.IDS_FeatureUsingTypeAlias.CheckFeatureAvailability(diagnostics, usingDirective.NamespaceOrType);
reportedError = !MessageID.IDS_FeatureUsingTypeAlias.CheckFeatureAvailability(diagnostics, usingDirective.NamespaceOrType);
}

var syntax = usingDirective.NamespaceOrType;
var flags = BinderFlags.SuppressConstraintChecks | BinderFlags.SuppressObsoleteChecks;
if (usingDirective.UnsafeKeyword != default)
{
this.CheckUnsafeModifier(DeclarationModifiers.Unsafe, usingDirective.UnsafeKeyword.GetLocation(), diagnostics);
if (!reportedError)
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
this.CheckUnsafeModifier(DeclarationModifiers.Unsafe, usingDirective.UnsafeKeyword.GetLocation(), diagnostics);

flags |= BinderFlags.UnsafeRegion;
}
else
Expand Down
Expand Up @@ -721,14 +721,39 @@ private UsingsAndDiagnostics GetUsingsAndDiagnostics(ref UsingsAndDiagnostics? u
continue;
}

var flags = BinderFlags.SuppressConstraintChecks;
if (usingDirective.UnsafeKeyword != default)
diagnostics.Add(ErrorCode.ERR_BadUnsafeInUsingDirective, usingDirective.UnsafeKeyword.GetLocation());
{
if (usingDirective.StaticKeyword == default)
{
diagnostics.Add(ErrorCode.ERR_BadUnsafeInUsingDirective, usingDirective.UnsafeKeyword.GetLocation());
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
// Make sure 'unsafe' is even allowed in this language version, reporting an error
// otherwise. If it is available, ensure that the user passed the '/unsafe' flag to
// to the compiler.
if (MessageID.IDS_FeatureUsingTypeAlias.CheckFeatureAvailability(diagnostics, usingDirective, usingDirective.UnsafeKeyword.GetLocation()))
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

@jcouv jcouv Mar 20, 2023

Choose a reason for hiding this comment

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

Same comment here. I would not make CheckUnsafeModifier conditional on langver check. Comment above will need to be updated accordingly #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

Choose a reason for hiding this comment

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

Was a test affected?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. many tests were affected by this.

declaringSymbol.CheckUnsafeModifier(DeclarationModifiers.Unsafe, usingDirective.UnsafeKeyword.GetLocation(), diagnostics);
}

flags |= BinderFlags.UnsafeRegion;
}
else
{
// Prior to C#12, allow the using static type to be an unsafe region. This allows us to
// maintain compat with prior versions of the compiler that allowed `using static
// List<int*[]>;` to be written. In 12.0 and onwards though, we require the code to
// explicitly contain the `unsafe` keyword.
if (!compilation.IsFeatureEnabled(MessageID.IDS_FeatureUsingTypeAlias))
flags |= BinderFlags.UnsafeRegion;
}

var directiveDiagnostics = BindingDiagnosticBag.GetInstance();
Debug.Assert(directiveDiagnostics.DiagnosticBag is object);
Debug.Assert(directiveDiagnostics.DependenciesBag is object);

declarationBinder ??= compilation.GetBinderFactory(declarationSyntax.SyntaxTree).GetBinder(usingDirective.NamespaceOrType).WithAdditionalFlags(BinderFlags.SuppressConstraintChecks);
declarationBinder ??= compilation.GetBinderFactory(declarationSyntax.SyntaxTree).GetBinder(usingDirective.NamespaceOrType).WithAdditionalFlags(flags);
var imported = declarationBinder.BindNamespaceOrTypeSymbol(usingDirective.NamespaceOrType, directiveDiagnostics, basesBeingResolved).NamespaceOrTypeSymbol;

if (imported.Kind == SymbolKind.Namespace)
Expand Down
7 changes: 6 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/SymbolExtensions.cs
Expand Up @@ -293,8 +293,13 @@ internal static void CheckUnsafeModifier(this Symbol symbol, DeclarationModifier
}

internal static void CheckUnsafeModifier(this Symbol symbol, DeclarationModifiers modifiers, Location errorLocation, BindingDiagnosticBag diagnostics)
=> CheckUnsafeModifier(symbol, modifiers, errorLocation, diagnostics.DiagnosticBag);

internal static void CheckUnsafeModifier(this Symbol symbol, DeclarationModifiers modifiers, Location errorLocation, DiagnosticBag? diagnostics)
{
if (((modifiers & DeclarationModifiers.Unsafe) == DeclarationModifiers.Unsafe) && !symbol.CompilationAllowsUnsafe())
if (diagnostics != null &&
(modifiers & DeclarationModifiers.Unsafe) == DeclarationModifiers.Unsafe &&
!symbol.CompilationAllowsUnsafe())
{
RoslynDebug.Assert(errorLocation != null);
diagnostics.Add(ErrorCode.ERR_IllegalUnsafe, errorLocation);
Expand Down
9 changes: 7 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 7 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 7 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 7 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 7 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 7 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 7 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ko.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 7 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pl.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 7 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pt-BR.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.