Track assemblies declaring well-known types used by a compilation. by AlekseyTs · Pull Request #40010 · dotnet/roslyn

AlekseyTs · 2019-11-25T21:24:16Z

Related to #37768.

AlekseyTs · 2019-11-26T17:47:13Z

@dotnet/roslyn-compiler Please review #Resolved

jcouv · 2019-11-26T18:25:03Z

                            diagnostics.Add(declaration.Type, useSiteDiagnostics);
                        }
-                        else if (TypeSymbol.Equals(previousType, Compilation.GetWellKnownType(WellKnownType.System_Exception), TypeCompareKind.ConsiderEverything2) &&
+                        else if (TypeSymbol.Equals(previousType, Compilation.GetWellKnownType(WellKnownType.System_Exception, recordUsage: false), TypeCompareKind.ConsiderEverything2) &&


ConsiderEverything2 [](start = 163, length = 19)

nit: when modifying a method, consider scrubbing ConsiderEverything2 (is ConsiderEverything the correct option?) #WontFix

is ConsiderEverything the correct option?

The option used here has no relation with the purpose of this PR and I do not intend spending any time thinking about it under umbrella of this PR.

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

I understand, but then I'm afraid we won't make progress on reviewing those incrementally, as every PR will likely raise similar pushback... Maybe worth discussing in team meeting (how do we make progress on those?), but unrelated to your PR. Thanks

In reply to: 350910134 [](ancestors = 350910134,350908701)

but then I'm afraid we won't make progress on reviewing those incrementally, as every PR will likely raise similar pushback...

There is a PR that touches 90 files across wide range of features and there is a PR that focuses on specific feature where a comparison actually plays a role.

In reply to: 350914019 [](ancestors = 350914019,350910134,350908701)

jcouv · 2019-11-26T18:29:33Z

            if ((!enumeratorType.IsSealed && !isAsync) ||
                this.Conversions.ClassifyImplicitConversionFromType(enumeratorType,
-                    isAsync ? this.Compilation.GetWellKnownType(WellKnownType.System_IAsyncDisposable) : this.Compilation.GetSpecialType(SpecialType.System_IDisposable),
+                    isAsync ? this.Compilation.GetWellKnownType(WellKnownType.System_IAsyncDisposable, recordUsage: false) : this.Compilation.GetSpecialType(SpecialType.System_IDisposable),


System_IAsyncDisposable [](start = 78, length = 23)

I assume we don't need to record this because we're also using IAsyncEnumerable or IAsyncEnumerator, which depend on this type, so will record usage? #Closed

I assume we don't need to record this because we're also using IAsyncEnumerable or IAsyncEnumerator, which depend on this type, so will record usage?

An attempt to classify conversion is not usage. Usage would be binding a conversion. In addition, we are using enumeratorType and for that type all bases (including implemented interfaces) are considered used.

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

jcouv · 2019-11-26T18:30:11Z


-                if (TypeSymbol.Equals(originalDefinition, compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_IAsyncEnumerable_T), TypeCompareKind.ConsiderEverything2) ||
-                    TypeSymbol.Equals(originalDefinition, compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_IAsyncEnumerator_T), TypeCompareKind.ConsiderEverything2))
+                if (TypeSymbol.Equals(originalDefinition, compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_IAsyncEnumerable_T, recordUsage: false), TypeCompareKind.ConsiderEverything2) ||


ConsiderEverything2 [](start = 185, length = 19)

nit: ConsiderEverything #WontFix

nit: ConsiderEverything

Not related to the change

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

jcouv · 2019-11-26T18:33:13Z

I have a general question on the scenarios/APIs/design: is it a requirement that we should be able to list used/unused assembly references without going to emit phase? My naive approach would have been to intercept references during emit phase, as it seems more centralized.
Or is the problem that we need to only record well-known types, and the emit phase doesn't distinguish those?

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

AlekseyTs · 2019-11-26T18:46:29Z

is it a requirement that we should be able to list used/unused assembly references without going to emit phase?

Emit is triggered by a special compilation API and it requires additional inputs not known to a Compilation. There is a requirement to make API available for analyzers, analyzers also don't have that information. Analyzers work during compilation and triggering at the same time yet another emit based on analyzer's request feels like a source of all kinds of problems.
Performance is another factor, full emit will take longer

For V1 of the feature we decided to track what we can track from binding and lowering.

In reply to: 558761495 [](ancestors = 558761495,558743543)

jcouv · 2019-11-26T18:48:11Z

Makes sense. Thanks!

In reply to: 558766520 [](ancestors = 558766520,558761495,558743543)

jcouv · 2019-11-26T18:57:58Z

            }

-            TypeSymbol securityAction = compilation.GetWellKnownType(WellKnownType.System_Security_Permissions_SecurityAction);
+            TypeSymbol securityAction = compilation.GetWellKnownType(WellKnownType.System_Security_Permissions_SecurityAction, recordUsage: false);


compilation.GetWellKnownType(WellKnownType.System_Security_Permissions_SecurityAction, recordUsage: false) [](start = 40, length = 106)

nit: I assume that some of the types don't record usage because they are referenced by other types which did record. This may be worth a comment? #Resolved

nit: I assume that some of the types don't record usage because they are referenced by other types which did record. This may be worth a comment?

Recorded below.

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

jcouv · 2019-11-26T19:04:46Z

        // Do the steps in compilation to get the method body diagnostics, but don't actually generate
        // IL or emit an assembly.
-        private void GetDiagnosticsForAllMethodBodies(DiagnosticBag diagnostics, CancellationToken cancellationToken)
+        private void GetDiagnosticsForAllMethodBodies(DiagnosticBag diagnostics, bool doLowering, CancellationToken cancellationToken)


doLowering [](start = 86, length = 10)

nit: This parameter probably could use a comment #Pending

nit: This parameter probably could use a comment

Please provide suggested wording beyond what the name already implies.

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

jcouv

LGTM Thanks (iteration 2)

333fred · 2019-11-27T15:33:32Z

                case ConversionKind.StackAllocToSpanType:
                    CheckFeatureAvailability(syntax, MessageID.IDS_FeatureRefStructs, diagnostics);
-                    stackAllocType = Compilation.GetWellKnownType(WellKnownType.System_Span_T).Construct(elementType);
+                    stackAllocType = Compilation.GetWellKnownType(WellKnownType.System_Span_T, recordUsage: false).Construct(elementType);


recordUsage: false [](start = 95, length = 18)

Why not record usage here? This seems like it will definitely be used by the final assembly? #Resolved

Why not record usage here? This seems like it will definitely be used by the final assembly?

An attempt to classify conversion is not a usage. It can be made by any external consumer through public API.

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

333fred · 2019-11-27T15:38:08Z

                TypeSymbol.Equals(
                    convertedArguments[0].Type,
-                    Compilation.GetWellKnownType(WellKnownType.System_Range),
+                    Compilation.GetWellKnownType(WellKnownType.System_Range, recordUsage: false),


recordUsage: false [](start = 77, length = 18)

Similar question: wouldn't we want to record usage here unless there are errors? #Resolved

Similar question: wouldn't we want to record usage here unless there are errors?

I don't think we should say that we use System.Range here if the convertedArguments[0].Type is not it. Do we report any error here if System.Range is missing? We use convertedArguments[0].Type and that will be tracked elsewhere by other means.

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

333fred · 2019-11-27T15:38:55Z

        {
            HashSet<DiagnosticInfo> useSiteDiagnostics = null;
-            TypeSymbol type = GetWellKnownType(wellKnownType, ref useSiteDiagnostics);
+            TypeSymbol type = GetWellKnownTypeWithoutRecordingUsage(wellKnownType, ref useSiteDiagnostics);


GetWellKnownTypeWithoutRecordingUsage [](start = 30, length = 37)

Everywhere else we've been using an optional parameter. Consider removing this method and just using that parameter. #WontFix

Everywhere else we've been using an optional parameter. Consider removing this method and just using that parameter.

This method never records usage, I see no point in adding any useless optional parameter.

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

333fred · 2019-11-27T15:40:12Z

            var result = TryImplicitConversionToArrayIndex(expr, type, node, attemptDiagnostics);
            if (result is object)
            {
+                if (!IsSemanticModelBinder)


IsSemanticModelBinder [](start = 21, length = 21)

We're checking/passing this condition in a bunch of different places. Consider adding a RecordUsedAssemblyIfNecessary on Binder that checks this in one place. #Resolved

We're checking/passing this condition in a bunch of different places. Consider adding a RecordUsedAssemblyIfNecessary on Binder that checks this in one place.

I am not sure I am following. It looks like there are only two if statements like this in binder code. I don't believe it is worth adding a helper for that.

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

It looks like there are only two if statements like this in binder code

The previous PR had 2 direct copies of this, plus 2 where this was checked in a nested local function (Binder.ResultSymbol). The symbol code also had a few cases where the symbol checked the property off the binder it was passed.

In reply to: 351437499 [](ancestors = 351437499,351359049)

And as we add tracking more scenarios, I imagine that will continue to expand.

In reply to: 351445110 [](ancestors = 351445110,351437499,351359049)

The previous PR had 2 direct copies of this, plus 2 where this was checked in a nested local function (Binder.ResultSymbol). The symbol code also had a few cases where the symbol checked the property off the binder it was passed.

I do not see this. There are only two places where we do if (!IsSemanticModelBinder) Compilation.AddUsedAssembly(...) and all of them are added in this PR. Binder.ResultSymbol doesn't even call Compilation.AddUsedAssembly. It is quite possible I still do not understand what exact refactoring you are suggesting. Are you suggesting to add another property to return !IsSemanticModelBinder from it and use it instead of checking IsSemanticModelBinder inline?

And as we add tracking more scenarios, I imagine that will continue to expand.

I do not plan to add any more similar tracking during binding.

In reply to: 351445297 [](ancestors = 351445297,351445110,351437499,351359049)

c5bc1e0#diff-a921499c9550563c162f3e1cbdff548bR387-R390
c5bc1e0#diff-56a949e9d40faeb9e94d55a560b7a6e5R197-R200

These aren't quite the same, but calling a very similar API with the same condition:
c5bc1e0#diff-591e192a1c591a08bec434a1c6a654abR318-R322
c5bc1e0#diff-591e192a1c591a08bec434a1c6a654abR335-R340
c5bc1e0#diff-3d0fdd05badb266fa634cddaee906ddeR197-R201

In reply to: 351451006 [](ancestors = 351451006,351445297,351445110,351437499,351359049)

The point being that, in general, I don't really think that IsSpeculativeSemanticModel is the type of check that it would occur to me that I need to perform before recording whether or not I'm using a type reference. I'm worried that in the future, someone will add code here that just adds the used assembly unconditionally, so it would be better to have that check made for you as part of a RecordIfNecessary API.

In reply to: 351453835 [](ancestors = 351453835,351451006,351445297,351445110,351437499,351359049)

so it would be better to have that check made for you as part of a RecordIfNecessary API.

Sorry, I do not see the benefit as anyone can call APIs on compilation directly. Again, it is quite possible that I am misinterpreting your suggestion. Perhaps you could provide the code for what is it you are suggesting.

In reply to: 351455345 [](ancestors = 351455345,351453835,351451006,351445297,351445110,351437499,351359049)

Perhaps we can follow up on this later in-person?

In reply to: 351457798 [](ancestors = 351457798,351455345,351453835,351451006,351445297,351445110,351437499,351359049)

333fred · 2019-11-27T15:55:15Z

+        /// This is fixed in 4.5 thus enabling block array initialization for a very common case.
+        /// We look for the presence of <see cref="System.Runtime.GCLatencyMode.SustainedLowLatency"/> which was introduced in .NET Framework 4.5
+        /// </remarks>
+        private bool EnableEnumArrayBlockInitialization


I'm not seeing what change requires this to move from the existing location? #Resolved

I'm not seeing what change requires this to move from the existing location?

There is only one consumer of this API, the emit layer. This makes it clear what it should do about tracking well-known types. The method is moved to avoid accidental use elsewhere in the future.

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

333fred · 2019-11-27T15:56:44Z

            }

-            var ctor = ((MethodSymbol)this._module.Compilation.GetWellKnownTypeMember(WellKnownMember.System_ReadOnlySpan_T__ctor));
+            var ctor = ((MethodSymbol)this._module.Compilation.GetWellKnownTypeMember(WellKnownMember.System_ReadOnlySpan_T__ctor, recordUsage: false));


Should we be recording usage if we successfully emit? #Resolved

Should we be recording usage if we successfully emit?

We don't track anything during emit, because the tracking is limited to binding and lowering, after that we return the result.

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

333fred · 2019-11-27T16:38:03Z

Done review pass (commit 2)

333fred

LGTM (commit 2), but I would like to talk more about refactoring the IsSemanticModelBinder check into a more centralized location.

Track assemblies declaring well-known types used by a compilation.

297b0ba

Related to dotnet#37768.

AlekseyTs added the Area-Compilers label Nov 25, 2019

AlekseyTs requested a review from a team November 25, 2019 21:24

Fix formatting

a16df68

jcouv reviewed Nov 26, 2019

View reviewed changes

jcouv approved these changes Nov 26, 2019

View reviewed changes

333fred reviewed Nov 27, 2019

View reviewed changes

333fred approved these changes Nov 27, 2019

View reviewed changes

AlekseyTs merged commit 6ef9ace into dotnet:features/UsedAssemblyReferences Nov 28, 2019

Conversation

AlekseyTs commented Nov 25, 2019

Uh oh!

AlekseyTs commented Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Nov 26, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 26, 2019 • 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!

jcouv Nov 26, 2019 • 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!

jcouv commented Nov 26, 2019

Uh oh!

AlekseyTs commented Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Nov 26, 2019

Uh oh!

jcouv Nov 26, 2019 • 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!

jcouv Nov 26, 2019 • 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!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

333fred Nov 27, 2019 • 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!

333fred Nov 27, 2019 • 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!

333fred Nov 27, 2019 • 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!

333fred Nov 27, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Nov 26, 2019 •

edited

Loading

jcouv Nov 26, 2019 •

edited by AlekseyTs

Loading

jcouv Nov 26, 2019 •

edited

Loading

jcouv Nov 26, 2019 •

edited by AlekseyTs

Loading

AlekseyTs commented Nov 26, 2019 •

edited

Loading

jcouv Nov 26, 2019 •

edited by AlekseyTs

Loading

jcouv Nov 26, 2019 •

edited by AlekseyTs

Loading

333fred Nov 27, 2019 •

edited by AlekseyTs

Loading

333fred Nov 27, 2019 •

edited by AlekseyTs

Loading

333fred Nov 27, 2019 •

edited by AlekseyTs

Loading

333fred Nov 27, 2019 •

edited by AlekseyTs

Loading

AlekseyTs Nov 27, 2019 •

edited

Loading

333fred Nov 27, 2019 •

edited by AlekseyTs

Loading

333fred Nov 27, 2019 •

edited by AlekseyTs

Loading