Remove the `bool IsUnsafe(this TypeSymbol type)` method #67831

jcouv · 2023-04-15T01:01:45Z

The first commit adds some tests as baseline (to show the impact of the change). This will be squashed, so you can ignore the TODO2 comments in that first commit :-P

The compiler had this confused helper method “IsUnsafe” which groups pointers, function pointers and arrays thereof together as “unsafe”.
It’s confused because either we only care about nested pointer types or we don’t, but this method cares about pointer arrays but misses a slew of nested scenarios (such as List<int*[]>).
I reviewed the places where it’s used and determined which should be relaxed (only care about top-level pointer types) and which should be tightened (to care about all nested pointer types).

There are two buckets:

Checks for unsafe context
a. Usings (which ignores pointer types before C# 12)
b. Invocations/new/base (reports any “unsafe” parameter type outside an unsafe context)
c. Method group conversions: D d = M; (fails if involves an “unsafe” type outside an unsafe context)
d. lambda parameter types: D d = (x) => x; (reports “unsafe” parameter types outside an unsafe context)
e. referencing an alias type
f. expressions
Special errors (disallowed)
a. anonymous types, records (members disallowed if “unsafe” type)
b. async/iterators (parameters disallowed if “unsafe” type)

This PR makes the following changes:

the first bucket corresponds to the language rule that pointer types don’t exist outside of unsafe context, so we tighten those (breaking change), except for using (per recent LDM discussion, because there was no way to declare an unsafe using)
the second bucket is about implementation constraints (can’t use pointers as type arguments for generic anonymous types, can’t compute their GetHashCode/Equals with our normal codegen), so those are relaxed

Update: note that unsafe code will remain disallowed in iterators ("An iterator block always defines a safe context, even when its declaration is nested in an unsafe context." is quoted in the code)

cston · 2023-04-21T23:07:31Z

src/Compilers/CSharp/Portable/Binder/Binder_AnonymousTypes.cs

@@ -223,7 +223,7 @@ private TypeSymbol GetAnonymousTypeFieldType(BoundExpression expression, CSharpS
                        errorArg = expressionType;
                        expressionType = CreateErrorType(SyntaxFacts.GetText(SyntaxKind.VoidKeyword));
                    }
-                    else if (expressionType.IsUnsafe())


IsUnsafe

I'm not sure when we get here with a void* but could we also get here with a void*[]? #Resolved

We get here with void* with a scenario like:

static void* M(void* b) { var a = new { F = b }; return a.F; }

That scenario remains an error (it's a pointer type).

On the other hand, scenarios with void*[] are now allowed:

var a = new { F = new void*[0] };

might be worthwhile to have a comment to this effect.

cston · 2023-04-21T23:16:47Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

@@ -1469,7 +1469,8 @@ static ErrorCode getRefMismatchErrorCode(TypeKind type)
                return true;
            }

-            if ((selectedMethod.HasUnsafeParameter() || selectedMethod.ReturnType.IsUnsafe()) && ReportUnsafeIfNotAllowed(syntax, diagnostics))
+            if ((selectedMethod.HasPointerOrFunctionPointerParameterType() || selectedMethod.ReturnType.IsPointerOrFunctionPointer())


selectedMethod.HasPointerOrFunctionPointerParameterType() || selectedMethod.ReturnType.IsPointerOrFunctionPointer()

These conditions are no longer including arrays of pointers. Were those cases not used previously? #Resolved

Those were used. See the UnsafeDelegateAssignment_* tests. The scenarios involving arrays of pointers were previously reported as error, but no longer are.

Are we testing constructed types such as delegate List<int*[]> D();? It looks like those cases should result in errors now.

jcouv · 2023-05-03T15:45:19Z

@cston @dotnet/roslyn-compiler Please take a look. Thanks

CyrusNajmabadi · 2023-05-03T15:59:12Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

        {
            if (!expr.HasAnyErrors && !IsInsideNameof)
            {
                TypeSymbol exprType = expr.Type;
-                if ((object)exprType != null && exprType.IsUnsafe())
+                if ((object)exprType != null && exprType.ContainsPointer())


Nit. "IsOrContainsPointerType" feels like a clearer name to me. But i can accept ContainsPointer as being clear enough as well. #Resolved

I'll keep ContainsPointer because that name aligns with other methods we have, like ContainsDynamic, ContainsNativeIntegerWrapperType, ContainsErrorType, etc

CyrusNajmabadi · 2023-05-03T16:08:50Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbolExtensions.cs

-                        return false;
-                }
-            }
-        }


👍 #Closed

jcouv · 2023-05-04T17:01:36Z

@cston @dotnet/roslyn-compiler Please take a look. Thanks

cston · 2023-05-04T20:36:10Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenFunctionPointersTests.cs

+                Diagnostic(ErrorCode.ERR_UnsafeNeeded, "B<delegate*<void>[]>").WithLocation(12, 12),
+                // (12,12): error CS0214: Pointers and fixed size buffers may only be used in an unsafe context
+                // [A<object>(B<delegate*<void>[]>.C)]
+                Diagnostic(ErrorCode.ERR_UnsafeNeeded, "B<delegate*<void>[]>.C").WithLocation(12, 12),


Just curious: Why are we reporting two errors for the type? #Resolved

The diagnostic for delegate* is reported in BindNamespaceOrTypeOrAliasSymbol (part of BindType).
The diagnostic for B<delegate*<void>[]> is reported in BindExpression (because it's on the left of a member access).
The diagnostic for B<delegate*<void>[]>.C is reported in BindExpression.

cston · 2023-05-04T20:50:23Z

src/Compilers/CSharp/Test/Semantic/Semantics/UnsafeTests.cs

+        }
+
+        [Fact]
+        public void ConstructorInitializerWithUnsafeArgument_PointerArray_Unsafe()


_Unsafe

Should this be "_Nested"? #Resolved

cston · 2023-05-04T20:53:46Z

src/Compilers/CSharp/Test/Symbol/Symbols/AnonymousTypesSemanticsTests.cs

+        }
+
+        [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/67330")]
+        public void AnonymousTypeSymbols_PointerListField()


AnonymousTypeSymbols_PointerListField

Is this test distinct from the previous test? #Resolved

AlekseyTs · 2023-05-09T20:12:25Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

            return expr;
        }

-        private void VerifyUnchecked(ExpressionSyntax node, BindingDiagnosticBag diagnostics, BoundExpression expr)
+        private void CheckContextForPointerTypes(ExpressionSyntax node, BindingDiagnosticBag diagnostics, BoundExpression expr)
        {
            if (!expr.HasAnyErrors && !IsInsideNameof)


if (!expr.HasAnyErrors && !IsInsideNameof)

Would skipping BoundTypeExpression nodes reduce the noise from redundant diagnostics? #Resolved

Possibly. That seems a good idea but I'm trying to keep this PR as small/focused as possible.

AlekseyTs · 2023-05-09T20:19:07Z

                // (9,44): error CS4005: Async methods cannot have unsafe parameters or return types

Should we adjust the wording of the message since we are relaxing the condition?

In reply to: 1540837450

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/BindingAsyncTests.cs:742 in 9eb0804. [](commit_id = 9eb0804, deletion_comment = True)

AlekseyTs · 2023-05-09T20:21:53Z

            // (4,66): error CS1637: Iterators cannot have unsafe parameters or yield types

Should we adjust the wording of this message?

In reply to: 1540840295

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/UnsafeTests.cs:965 in 9eb0804. [](commit_id = 9eb0804, deletion_comment = True)

AlekseyTs · 2023-05-09T20:29:51Z

src/Compilers/CSharp/Portable/Binder/ExecutableCodeBinder.cs

@@ -113,7 +113,7 @@ public static void ValidateIteratorMethod(CSharpCompilation compilation, MethodS
                {
                    diagnostics.Add(ErrorCode.ERR_BadIteratorArgType, parameter.GetFirstLocation());
                }
-                else if (parameter.Type.IsUnsafe())
+                else if (parameter.Type.IsPointerOrFunctionPointer())
                {
                    diagnostics.Add(ErrorCode.ERR_UnsafeIteratorArgType, parameter.GetFirstLocation());


diagnostics.Add(ErrorCode.ERR_UnsafeIteratorArgType, parameter.GetFirstLocation());

It looks like this error mentions return type as well, but I do not see where we check the return type. Are we missing the check, or the wording is wrong? #Resolved

This question is relevant if we decide to adjust the wording of the message.

For iterators, yielding a pointer type is indeed forbidden (pointer types cannot be used as type arguments, guarded with normal "error CS0306: The type '...' may not be used as a type argument").
I agree that this error message doesn't need to mention yielded types, but it also seems fine to leave as-is (the message is not wrong) and it's tangential to this PR.

AlekseyTs · 2023-05-09T20:32:24Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbol.cs

@@ -66,7 +66,7 @@ internal void ReportAsyncParameterErrors(BindingDiagnosticBag diagnostics, Locat
                {
                    diagnostics.Add(ErrorCode.ERR_BadAsyncArgType, getLocation(parameter, location));
                }
-                else if (parameter.Type.IsUnsafe())
+                else if (parameter.Type.IsPointerOrFunctionPointer())
                {
                    diagnostics.Add(ErrorCode.ERR_UnsafeAsyncArgType, getLocation(parameter, location));


diagnostics.Add(ErrorCode.ERR_UnsafeAsyncArgType, getLocation(parameter, location));

It looks like this error mentions return type as well, but I do not see where we check the return type. Are we missing the check, or the wording is wrong? #Resolved

This question is relevant if we decide to adjust the wording of the message.

The same applies here. Task/ValueTask/MyTask<pointer> is forbidden because of type argument restrictions. The message doesn't need to mention return types and arguably could be better if it didn't, but I don't plan to change it.

AlekseyTs · 2023-05-09T21:05:40Z

src/Compilers/CSharp/Test/Symbol/Compilation/UsedAssembliesTests.cs

            {
                var references = new[] { reference0, reference1 };
-                AssertUsedAssemblyReferences(source, references, references, parseOptions: TestOptions.Regular11);


AssertUsedAssemblyReferences

It looks like the intent here was to test a success scenario. So, I suggest to enable unsafe code for it and keep testing a success scenario #Resolved

I guess, it is fine to leave as is if the test below does exactly that.

That was my feeling too

AlekseyTs · 2023-05-09T21:14:52Z

src/Compilers/CSharp/Test/Symbol/Symbols/AnonymousTypesSemanticsTests.cs

+}
+""";
+            var comp = CreateCompilation(source, options: TestOptions.UnsafeDebugDll);
+            comp.VerifyDiagnostics();


comp.VerifyDiagnostics();

Would it be worth verifying that we are actually able to execute the code? #Closed

AlekseyTs · 2023-05-09T21:20:43Z

Done with review pass (commit 8)

jcouv · 2023-05-10T01:37:20Z

                // (9,44): error CS4005: Async methods cannot have unsafe parameters or return types

I didn't follow. The PR tightens the enforcement that pointer types do not exist outside of unsafe context (we're reporting more instances of ERR_UnsafeNeeded than we did before).

In reply to: 1540837450

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/BindingAsyncTests.cs:742 in 9eb0804. [](commit_id = 9eb0804, deletion_comment = True)

jcouv · 2023-05-10T01:46:52Z

            // (4,66): error CS1637: Iterators cannot have unsafe parameters or yield types

That code didn't change and the error still seems correct. I couldn't dig the spec referenced in our code, but this PR doesn't aim to change the rule we use:

// Spec 8.2: "An iterator block always defines a safe context, even when its declaration
// is nested in an unsafe context."
Error(diagnostics, ErrorCode.ERR_IllegalInnerUnsafe, node.UnsafeKeyword);

I'll add a note of clarification to description in OP (pointer scenarios remain disallowed in iterators because their context is always safe).

In reply to: 1540840295

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/UnsafeTests.cs:965 in 9eb0804. [](commit_id = 9eb0804, deletion_comment = True)

AlekseyTs · 2023-05-10T03:39:31Z

                // (9,44): error CS4005: Async methods cannot have unsafe parameters or return types

I didn't follow. The PR tightens the enforcement that pointer types do not exist outside of unsafe context (we're reporting more instances of ERR_UnsafeNeeded than we did before).

Sorry, it looks like CodeFlow failed to place the comment properly. I you look at the diff between commits 1 and 9 for test UnsafeAsyncArgType_PointerArray, there is a bunch of errors removed like the one below:

// (6,40): error CS4005: Async methods cannot have unsafe parameters or return types
//     unsafe async static Task M1(int*[] i) { await Task.Yield(); } // 1
Diagnostic(ErrorCode.ERR_UnsafeAsyncArgType, "i").WithLocation(6, 40),

It looks like we relaxed condition for ERR_UnsafeAsyncArgType. The term "unsafe parameters or return types" now feels misleading. Perhaps we should say "pointer types" instead.

In reply to: 1541163321

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/BindingAsyncTests.cs:742 in 9eb0804. [](commit_id = 9eb0804, deletion_comment = True)

AlekseyTs · 2023-05-10T03:49:52Z

src/Compilers/CSharp/Test/Semantic/Semantics/UnsafeTests.cs

@@ -944,6 +950,44 @@ public void UnsafeIteratorSignatures()
                Diagnostic(ErrorCode.ERR_IllegalInnerUnsafe, "Iterator")); //this is for putting "unsafe" on an iterator, not for the parameter type
        }

+        [Fact]
+        public void UnsafeIteratorSignatures_PointerArray()


UnsafeIteratorSignatures_PointerArray

The other comment was placed by CodeFlow in a wrong place. In the test UnsafeIteratorSignatures_PointerArray (commit 1 vs. 9), we are removing:

// (4,66): error CS1637: Iterators cannot have unsafe parameters or yield types // System.Collections.Generic.IEnumerable<int> Iterator(int*[] p) Diagnostic(ErrorCode.ERR_UnsafeIteratorArgType, "p").WithLocation(4, 66)

Perhaps the unsafe is a misleading term to use in the message now. The check is specifically about pointer types now, and unsafe types in a more general term are not disallowed. #Closed

jcouv · 2023-05-10T15:13:40Z

Thanks for the clarification. Updated the uses of "unsafe" in those two diagnostics.

AlekseyTs

LGTM (commit 10), assuming CI is passing

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 15, 2023

jcouv self-assigned this Apr 15, 2023

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 15, 2023

jcouv force-pushed the is-unsafe branch from e75d08f to c187da3 Compare April 15, 2023 01:36

Baseline for IsUnsafe work

9eb0804

jcouv force-pushed the is-unsafe branch from c187da3 to 7d74a7c Compare April 15, 2023 01:48

build-analysis bot mentioned this pull request Apr 15, 2023

NuGet failing with Response status code does not indicate success: 503 (Service Unavailable) dotnet/arcade#11723

Open

5 tasks

fix

d163abb

jcouv force-pushed the is-unsafe branch from 7d74a7c to d163abb Compare April 15, 2023 04:34

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 15, 2023

jcouv added this to the 17.7 milestone Apr 15, 2023

jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Apr 15, 2023

jcouv marked this pull request as ready for review April 15, 2023 15:36

jcouv requested a review from a team as a code owner April 15, 2023 15:36

cston reviewed Apr 21, 2023

View reviewed changes

jcouv marked this pull request as draft April 24, 2023 16:24

jcouv force-pushed the is-unsafe branch 4 times, most recently from 4198a4d to a69f097 Compare May 1, 2023 05:38

Use stricter rules

c460621

jcouv force-pushed the is-unsafe branch from a69f097 to c460621 Compare May 1, 2023 05:43

jcouv added 3 commits May 1, 2023 08:27

Merge remote-tracking branch 'dotnet/main' into is-unsafe

0d46b96

Merge conflict

ddb6f97

tweaks

0275f64

jcouv force-pushed the is-unsafe branch from 2b084e7 to 0275f64 Compare May 1, 2023 19:13

jcouv marked this pull request as ready for review May 2, 2023 06:17

Tweak breaking change doc

156a695

CyrusNajmabadi reviewed May 3, 2023

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/TypeSymbolExtensions.cs

return false;

}

}

}

Copy link

Member

CyrusNajmabadi May 3, 2023 •

edited by jcouv

Loading

Choose a reason for hiding this comment

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

👍 #Closed

cston reviewed May 4, 2023

View reviewed changes

cston approved these changes May 4, 2023

View reviewed changes

Address feedback

5353df7

AlekseyTs reviewed May 9, 2023

View reviewed changes

Execute test

9800359

jcouv requested a review from AlekseyTs May 10, 2023 01:47

AlekseyTs reviewed May 10, 2023

View reviewed changes

Update diagnostic messages

b4080ee

AlekseyTs approved these changes May 10, 2023

View reviewed changes

jcouv merged commit e123745 into dotnet:main May 10, 2023

jcouv deleted the is-unsafe branch May 10, 2023 18:10

allisonchou mentioned this pull request May 12, 2023

[Automated] PRs inserted in VS build main-33712.304 #68184

Closed

Remove the bool IsUnsafe(this TypeSymbol type) method #67831

Remove the bool IsUnsafe(this TypeSymbol type) method #67831

Conversation

jcouv commented Apr 15, 2023 • edited Loading

cston Apr 21, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Apr 21, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented May 3, 2023

CyrusNajmabadi May 3, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi May 3, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv commented May 4, 2023

cston May 4, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston May 4, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

cston May 4, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs May 9, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 9, 2023 • edited Loading

AlekseyTs commented May 9, 2023 • edited by jcouv Loading

AlekseyTs May 9, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 9, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 9, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 9, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented May 9, 2023

jcouv commented May 10, 2023

jcouv commented May 10, 2023

AlekseyTs commented May 10, 2023

AlekseyTs May 10, 2023 • edited Loading

Choose a reason for hiding this comment

jcouv commented May 10, 2023

AlekseyTs left a comment

Choose a reason for hiding this comment

Remove the `bool IsUnsafe(this TypeSymbol type)` method #67831

Remove the `bool IsUnsafe(this TypeSymbol type)` method #67831

jcouv commented Apr 15, 2023 •

edited

Loading

cston Apr 21, 2023 •

edited by jcouv

Loading

cston Apr 21, 2023 •

edited by jcouv

Loading

CyrusNajmabadi May 3, 2023 •

edited by jcouv

Loading

CyrusNajmabadi May 3, 2023 •

edited by jcouv

Loading

cston May 4, 2023 •

edited by jcouv

Loading

cston May 4, 2023 •

edited by jcouv

Loading

cston May 4, 2023 •

edited by jcouv

Loading

AlekseyTs May 9, 2023 •

edited by jcouv

Loading

AlekseyTs commented May 9, 2023 •

edited

Loading

AlekseyTs commented May 9, 2023 •

edited by jcouv

Loading

AlekseyTs May 9, 2023 •

edited by jcouv

Loading

AlekseyTs May 9, 2023 •

edited by jcouv

Loading

AlekseyTs May 9, 2023 •

edited by jcouv

Loading

AlekseyTs May 9, 2023 •

edited

Loading

AlekseyTs May 10, 2023 •

edited

Loading