Add use site diagnostics to IsUnmanaged by RikkiGibson · Pull Request #46114 · dotnet/roslyn

RikkiGibson · 2020-07-18T00:08:50Z

jcouv · 2020-07-21T21:09:53Z

                hasErrors = CheckManagedAddr(Compilation, operandType, node.Location, diagnostics);

Are we reporting duplicate diagnostics here?
Maybe we could avoid duplicates by doing something like if (!allowManagedAddressOf) { ... } else if (!hasErrors) { diagnostics.Add(node.Location, useSiteDiagnostics); }

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:2249 in 497fb0e. [](commit_id = 497fb0e, deletion_comment = False)

jcouv · 2020-07-21T21:10:48Z

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncMethodToStateMachineRewriter.cs

                {
                    continue;
                }
+                F.Diagnostics.Add(hoistedLocal.Locations.FirstOrNone(), useSiteDiagnostics);


do we have a test for this case?

No, although looking this scenario, it seems like the only drawback of the missing references is that we zero out a field that might not need to be zeroed out (i.e. if it is of an unmanaged type). Therefore I am considering dropping the use site diagnostics in this scenario.

I decided to keep the use site diagnostics here. It might end up being a breaking change, though.

jcouv

Done with review pass (iteration 2)

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

AlekseyTs · 2020-07-21T21:20:33Z

                hasErrors = CheckManagedAddr(Compilation, operandType, node.Location, diagnostics);

Are we reporting duplicate diagnostics here?

Usually, we aren't concerned much about reporting duplicate use-site diagnostics, if that is the case here.

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

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:2249 in 497fb0e. [](commit_id = 497fb0e, deletion_comment = False)

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

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncMethodToStateMachineRewriter.cs

AlekseyTs · 2020-07-21T21:57:33Z

                hasErrors = CheckManagedAddr(Compilation, operandType, node.Location, diagnostics);

I think I see what is the concern. The CheckManagedAddr gets ManagedKind, which effectively going to report the same use-site diagnostics. I think we could avoid this with small refactoring.

Here we calculate ManagedKind
We add a overload of CheckManagedAddr that takes ManagedKind already calcualated, old overload delegates to new one.
We call the new overload here, thus, avoiding calculating the same information twice.

In reply to: 662112597 [](ancestors = 662112597,662108031)

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:2249 in 497fb0e. [](commit_id = 497fb0e, deletion_comment = False)

AlekseyTs · 2020-07-21T22:06:58Z

Done with review pass (iteration 2) #Closed

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

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

AlekseyTs · 2020-07-22T16:52:16Z

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

+}", assemblyName: "libS1").ToMetadataReference();
+
+
+        private static MetadataReference UnmanagedUseSiteError_Ref2 = CreateCompilation(@"


static [](start = 16, length = 6)

readonly? #Closed

AlekseyTs · 2020-07-22T16:57:45Z

Done with review pass (iteration 3) #Closed

AlekseyTs

LGTM (iteration 5)

RikkiGibson · 2020-07-23T02:22:57Z

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

-                    return baseKind;
-                }
-                return managedKind;
+                Debug.Assert(_managedKindUseSiteDiagnostics.IsDefault);


This assertion does not pass reliably, probably because it is essentially an assertion that this thread is the first one to try and populate the ManagedKind on this symbol. This code works by basically letting threads that race to initialize this value overwrite one another, so the assertion is not actually valid.

AlekseyTs · 2020-07-24T17:48:45Z

@RikkiGibson Is something holding this PR other than the second sign-off? Consider pushing this PR through sooner rather than later. I am planning to do an integration to UsedAssemblyReferences branch soon.

RikkiGibson · 2020-07-24T20:06:53Z

This is ready to merge once we get a second sign-off. Please review @dotnet/roslyn-compiler

jaredpar · 2020-07-24T20:08:12Z

Looking. Got interrupted earlier. Will try and finish soonish.

jaredpar · 2020-07-24T20:12:45Z

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

                        continue;
                    }

+                    fieldType.AddUseSiteDiagnostics(ref useSiteDiagnostics);


Does this mean that we will get say obsolete warnings when we use a struct in a place where we checked the unmanaged constraint? #Resolved

Does this mean that we will get say obsolete warnings when we use a struct in a place where we checked the unmanaged constraint?

Obsolete warnings are not reported through the use-site diagnostics mechanism.

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

Reference unification warnings could be reported

In reply to: 460271139 [](ancestors = 460271139,460266173)

jaredpar · 2020-07-24T20:14:05Z

src/Compilers/CSharp/Portable/Symbols/PublicModel/TypeSymbol.cs

        }

-        bool ITypeSymbol.IsUnmanagedType => !UnderlyingTypeSymbol.IsManagedType;
+        bool ITypeSymbol.IsUnmanagedType => !UnderlyingTypeSymbol.IsManagedTypeNoUseSiteDiagnostics;


Seems like there is a gap now between what we have interntally and what we expose through the public API. Is there a need to give this info to our API consumers? #Resolved

Seems like there is a gap now between what we have interntally and what we expose through the public API. Is there a need to give this info to our API consumers?

We have a bunch of API like that. We do not expose use-site diagnostics or any other diagnostics for any of them. They do not return diagnostics. APIs that return diagnostics will incorporate use-site diagnostics naturally.

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

jaredpar · 2020-07-24T20:16:18Z

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

+                _flags.SetManagedKind(managedKind);
            }
+
+            Thread.MemoryBarrier();


What protection do you believe this is giving you here? #Resolved

Does this mean that we will get say obsolete warnings when we use a struct in a place where we checked the unmanaged constraint?

The intent is to ensure that below we are dealing with the latest value of the _managedKindUseSiteDiagnostics field, since we are reading and updating two fields separately.

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

Don't think the MemoryBarrier will provide that. The barrier controls read / write reordering from within a thread. It doesn't do anything to guarantee the synchronization of writes from other threads for this thread.

Stated another way. Lets say Thread 1 is the one which enters the if (managedKind ==) above and T2 skips the if and goes straight to this line. In T1 the order of writes is indeed _managedKindUseSiteDiagnostics and then _flags. That does not mean the order in which the writes become visible to T2 is the same. The order in which the writes become visible could be _flags then _managedKindUseSiteDiagnostics. The MemoryBarrier doesn't help here because all it does is control the order of read / writes within T2. It doesn't control whether or not those reads are consistent with the writse from T1.

It's possible I'm remembering part of this wrong but pretty much whenever u have to depend on the ordering of writes between threads there is usually this type of gap.

The barrier controls read / write reordering from within a thread.

That is what we are going after. Not any syncronization

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

I think there is a race then. Even if we use the barrier to ensure the thread can't reorder it's reads of _flags and _managedKindUseSiteDiagnostics we can't guarantee the visibility of the writes from the other thread are both visible.

It's possible I'm remembering part of this wrong but pretty much whenever u have to depend on the ordering of writes between threads there is usually this type of gap.

I think ideally we need a volatile read here, but, I guess we don't have one, for an immutable array

I think there is a race then. Even if we use the barrier to ensure the thread can't reorder it's reads of _flags and _managedKindUseSiteDiagnostics we can't guarantee the visibility of the writes from the other thread are both visible.

I was suggesting an alternative to check both values in the if above, this would guarantee that we have non-default value here

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

Agree I think either of those would work.

jaredpar · 2020-07-24T20:16:41Z

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

+            get
+            {
+                HashSet<DiagnosticInfo>? useSiteDiagnostics = null;
+                return GetManagedKind(ref useSiteDiagnostics);


Wish we had done ref var in the same way we did out var

It's not too late! For C# Next that is.

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

jaredpar

Good once we merge the changes to th

…-pointers * upstream/master: (207 commits) Update argument state when parameter has not-null type (dotnet#46072) Fix TypeWithAnnotations.ToTypeWithState() for (untyped) null literal (dotnet#46344) Update README (dotnet#46136) Revert "Revert "Support nullable annotations on unconstrained type parameters"" Revert "Support nullable annotations on unconstrained type parameters (dotnet#45993)" Fix type in publish data Update VSIXExpInstaller version to one available on ADO Update publish data for 16.8 Update version of RichCodeNav.EnvVarDump A fixed initializer must be bound to its natural type (dotnet#46293) Update features merged into 16.7p4 (dotnet#46229) Async-streams: disposal should continue without jump within a finally (dotnet#46188) Recommend default in type constraint, but not record (dotnet#46311) Add use site diagnostics to IsUnmanaged (dotnet#46114) Add another flaky test. Ensure NuGet connections use TLS 1.2 Update to Microsoft.CodeAnalysis.Testing 1.0.1-beta1.20374.2 Skip flaky test. Fix build break. (dotnet#46303) Skip a flaky test Relates to dotnet#46304 ...

…to function-pointer-type-lookup * upstream/features/function-pointers: (212 commits) Correct public API number. Update argument state when parameter has not-null type (dotnet#46072) Fix TypeWithAnnotations.ToTypeWithState() for (untyped) null literal (dotnet#46344) Update README (dotnet#46136) Revert "Revert "Support nullable annotations on unconstrained type parameters"" Revert "Support nullable annotations on unconstrained type parameters (dotnet#45993)" Fix type in publish data Update VSIXExpInstaller version to one available on ADO Update publish data for 16.8 Update version of RichCodeNav.EnvVarDump A fixed initializer must be bound to its natural type (dotnet#46293) Update features merged into 16.7p4 (dotnet#46229) Async-streams: disposal should continue without jump within a finally (dotnet#46188) Recommend default in type constraint, but not record (dotnet#46311) Add use site diagnostics to IsUnmanaged (dotnet#46114) Add another flaky test. Ensure NuGet connections use TLS 1.2 Update to Microsoft.CodeAnalysis.Testing 1.0.1-beta1.20374.2 Skip flaky test. Fix build break. (dotnet#46303) ...

RikkiGibson added the Area-Compilers label Jul 18, 2020

RikkiGibson force-pushed the isunmanaged-usesitediagnostics branch from 176f019 to 9eb4baa Compare July 18, 2020 02:30

Add use site diagnostics to IsUnmanaged

4a10da7

RikkiGibson force-pushed the isunmanaged-usesitediagnostics branch from 9eb4baa to 4a10da7 Compare July 18, 2020 02:46

Add unmanaged use site error tests

497fb0e

RikkiGibson marked this pull request as ready for review July 20, 2020 21:07

RikkiGibson requested a review from a team as a code owner July 20, 2020 21:07

jcouv reviewed Jul 21, 2020

View reviewed changes

jcouv self-assigned this Jul 21, 2020

AlekseyTs reviewed Jul 21, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/ConstraintsHelper.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jul 21, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs Show resolved Hide resolved

AlekseyTs reviewed Jul 21, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncMethodToStateMachineRewriter.cs Outdated Show resolved Hide resolved

Address feedback

6c49481

AlekseyTs reviewed Jul 22, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jul 22, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jul 22, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jul 22, 2020

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/UseSiteErrorTests.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jul 22, 2020

View reviewed changes

RikkiGibson added 2 commits July 22, 2020 11:04

Address feedback

1986ec4

Add Thread.MemoryBarrier call to ensure read order

8c585ea

AlekseyTs approved these changes Jul 22, 2020

View reviewed changes

RikkiGibson commented Jul 23, 2020

View reviewed changes

Remove invalid assertion

a916763

RikkiGibson mentioned this pull request Jul 23, 2020

Flaky test RemoteHostClientServiceFactoryTests.UpdaterService #46252

Closed

jaredpar reviewed Jul 24, 2020

View reviewed changes

RikkiGibson commented Jul 24, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs Outdated Show resolved Hide resolved

RikkiGibson commented Jul 24, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs Outdated Show resolved Hide resolved

Apply suggestions from code review

881ba93

jaredpar approved these changes Jul 24, 2020

View reviewed changes

AlekseyTs merged commit c2faa09 into dotnet:master Jul 25, 2020

ghost added this to the Next milestone Jul 25, 2020

RikkiGibson deleted the isunmanaged-usesitediagnostics branch July 25, 2020 02:04

RikkiGibson modified the milestones: Next, 16.8.P2 Aug 11, 2020

jaredpar mentioned this pull request Mar 1, 2021

The type 'PartitionKeyInternal' is defined in an assembly that is not referenced. Azure/azure-cosmos-dotnet-v3#2264

Open

		}", assemblyName: "libS1").ToMetadataReference();


		private static MetadataReference UnmanagedUseSiteError_Ref2 = CreateCompilation(@"

Conversation

RikkiGibson commented Jul 18, 2020

Uh oh!

jcouv commented Jul 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlekseyTs commented Jul 21, 2020

Uh oh!

Uh oh!

Uh oh!

AlekseyTs commented Jul 21, 2020

Uh oh!

AlekseyTs commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlekseyTs Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jul 24, 2020

Uh oh!

RikkiGibson commented Jul 24, 2020

Uh oh!

jaredpar commented Jul 24, 2020

Uh oh!

jaredpar Jul 24, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Jul 24, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Jul 24, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jul 21, 2020 •

edited

Loading

AlekseyTs Jul 22, 2020 •

edited

Loading

AlekseyTs commented Jul 22, 2020 •

edited

Loading

jaredpar Jul 24, 2020 •

edited by AlekseyTs

Loading

jaredpar Jul 24, 2020 •

edited by AlekseyTs

Loading

jaredpar Jul 24, 2020 •

edited by AlekseyTs

Loading

jaredpar Jul 24, 2020 •

edited

Loading