Beginning of work on "Used Assembly References" feature. by AlekseyTs · Pull Request #39807 · dotnet/roslyn

AlekseyTs · 2019-11-13T23:50:04Z

This is a beginning of work on #37768.

Detect used assembly references from:

explicit references to types in source;
explicit references to namespaces in source;
explicit references to fields in source;
explicit method invocations in source;

Usings flagged by the compiler as unused shouldn’t contribute to the set of used references.

Detect used assembly references from: - explicit references to types in source; - explicit references to namespaces in source; - explicit references to fields in source; - explicit method invocations in source; Usings flagged by the compiler as unused shouldn’t contribute to the set of used references.

AlekseyTs · 2019-11-14T18:53:01Z

@gafter, @dotnet/roslyn-compiler Please review.

333fred · 2019-11-14T21:39:13Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

+        /// For example, if a type declared in a referenced assembly is referenced in source code 
+        /// within this compilation, the reference is considered to be used. Etc.
+        /// The returned set is a subset of references returned by <see cref="References"/> API.
+        /// If compilation contains errors, it is valid to consider all assembly references as used for the purpose


If compilation [](start = 12, length = 14)

Nit: If the compilation #Resolved

gafter · 2019-11-15T19:03:43Z

docs/features/UsedAssemblyReferences.md

+=========================
+
+The *Used Assembly References* feature provides a ```GetUsedAssemblyReferences``` API on a ```Compilation``` to obtain a set
+of metadata assembly references that are considered as used by the compilation. For example, if a type declared in a


as [](start = 52, length = 2)

"as" should be "to be". Also in the line below. #Closed

gafter · 2019-11-15T19:04:19Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

        internal abstract void GetDiagnostics(CompilationStage stage, bool includeEarlierStages, DiagnosticBag diagnostics, CancellationToken cancellationToken = default);

+        /// <summary>
+        /// Unique metadata assembly references that are considered as used by this compilation.


as [](start = 68, length = 2)

"to be" instead of "as" here and four lines below. #Closed

gafter · 2019-11-15T19:19:00Z

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

+                    {
+                        NamedTypeSymbol container = symbol.ContainingType;
+
+                        if (container is null)


if [](start = 24, length = 2)

Can you please explain why this whole if statement isn't just Compilation.AddAssembliesUsedByTypeReference((TypeSymbol)symbol);? #Closed

Can you please explain why this whole if statement isn't just Compilation.AddAssembliesUsedByTypeReference((TypeSymbol)symbol);?

I guess it could be. We would unnecessary visit type arguments for the type, but it is probably not a big deal. I will simplify.

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

gafter · 2019-11-15T19:22:52Z

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

+                    if (imported is NamespaceSymbol ns)
+                    {
+                        Debug.Assert(!ns.IsGlobalNamespace);
+                        compilation.AddAssembliesUsedByNamespaceReference(ns);


compilation [](start = 24, length = 11)

Is this line tested? #Closed

Is this line tested?

Yes.

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

gafter · 2019-11-15T19:36:21Z

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

+
+            try
+            {
+                var visitor = new UsedAssembliesRecorder(compilation);


new UsedAssembliesRecorder(compilation) [](start = 30, length = 39)

It feels like it would be valuable to cache one UsedAssembliesRecorder per compilation (e.g. in the compilation). #Closed

It feels like it would be valuable to cache one UsedAssembliesRecorder per compilation (e.g. in the compilation).

The base class has state which cannot be shared while walking nodes in parallel.

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

AlekseyTs · 2019-11-15T19:38:45Z

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

+                // PROTOTYPE(UsedAssemblyReferences): This error might not be cached, but its presence might affect cached full set of used assemblies. 
+                //                                    We would report all assemblies as used, even though no one will ever see this error and under
+                //                                    different environment state the pass could succeed causing us to return different set of used assemblies
+                //                                    with now apparent reason for the difference from the consumer's point of view. 


now [](start = 59, length = 3)

Typo #Closed

gafter · 2019-11-15T19:52:45Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

+        /// For example, if a type declared in a referenced assembly is referenced in source code 
+        /// within this compilation, the reference is considered to be used. Etc.
+        /// The returned set is a subset of references returned by <see cref="References"/> API.
+        /// If compilation contains errors, it is valid to consider all assembly references as used for the purpose


If [](start = 12, length = 2)

I would suggest replacing these two sentences with "The result is undefined if the compilation contains errors." #Closed

gafter · 2019-11-15T19:54:03Z

Completed review of Iteration 1. #Closed

AlekseyTs · 2019-11-18T17:48:25Z

@dotnet/roslyn-compiler Please review.

333fred · 2019-11-18T21:32:41Z

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

+                                            {
+                                                if (!underlyingDependency.IsLinked && usedAssemblies.Contains(underlyingDependency))
+                                                {
+                                                    AssemblySymbol dependency;


NIt: could use inline declaration #WontFix

333fred · 2019-11-18T21:34:30Z

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

+                return false;
+            }
+
+            _lazyUsedAssemblyReferences ??= new ConcurrentSet<AssemblySymbol>();


_lazyUsedAssemblyReferences [](start = 12, length = 27)

Given that we're locking on this later, I don't think ??= provides the correct threading guarantee here. We probably need to use Interlocked.CompareExchange. #Closed

333fred · 2019-11-18T21:50:49Z

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

+
+namespace Microsoft.CodeAnalysis.CSharp
+{
+    internal sealed class UsedAssembliesRecorder : BoundTreeWalkerWithStackGuard


What about:

typeof

default

events

exception variable declarations #Closed

What about:

I believe PR description clearly says what is in scope for this PR. BTW, typeof and "exception variable declarations" do not need special handling because the types are referred to explicitly in syntax and will be intercepted by Binder.ResultSymbol.

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

333fred · 2019-11-18T21:56:59Z

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

+class C2
+{
+    /// <summary>
+    /// <see cref=""C1{C0}.E1.F1""/>


C0 [](start = 23, length = 2)

I'm not understanding why this wouldn't count as a reference to comp0 when parsing documentation? #Closed

I'm not understanding why this wouldn't count as a reference to comp0 when parsing documentation?

Because it is treated as a definition of a type parameter, that could be referred to later. You don't see cref parsing working this way?

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

333fred · 2019-11-18T22:03:48Z

src/Compilers/VisualBasic/Portable/Compilation/VisualBasicCompilation.vb

        End Property

+        Friend Overrides Function GetUsedAssemblyReferences(Optional cancellationToken As CancellationToken = Nothing) As ImmutableArray(Of MetadataReference)
+            Throw New System.NotImplementedException()


Is this planned on being implemented for VB? #Closed

Is this planned on being implemented for VB?

Yes, I'll add a prototype comment.

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

333fred · 2019-11-18T22:05:12Z

Done review pass (commit 3) #Resolved

333fred

LGTM (commit 4)

gafter

AlekseyTs · 2019-11-19T18:10:23Z

@ivanbasov FYI

AlekseyTs added the Area-Compilers label Nov 13, 2019

AlekseyTs requested a review from a team as a code owner November 13, 2019 23:50

333fred reviewed Nov 14, 2019

View reviewed changes

gafter reviewed Nov 15, 2019

View reviewed changes

AlekseyTs commented Nov 15, 2019

View reviewed changes

gafter reviewed Nov 15, 2019

View reviewed changes

AlekseyTs added 2 commits November 15, 2019 14:24

PR feedback

b558615

More tests

259b7ea

333fred reviewed Nov 18, 2019

View reviewed changes

PR feedback

7ba0c61

333fred approved these changes Nov 19, 2019

View reviewed changes

gafter approved these changes Nov 19, 2019

View reviewed changes

AlekseyTs merged commit c5bc1e0 into dotnet:features/UsedAssemblyReferences Nov 19, 2019

jmarolf mentioned this pull request Feb 7, 2022

Add repo files to Roslyn.sln and Compilers.sln as solution items #59328

Closed

Conversation

AlekseyTs commented Nov 13, 2019

Uh oh!

AlekseyTs commented Nov 14, 2019

Uh oh!

333fred Nov 14, 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!

gafter Nov 15, 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!

gafter Nov 15, 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!

gafter Nov 15, 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!

AlekseyTs Nov 15, 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!

gafter Nov 15, 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!

AlekseyTs Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

gafter Nov 15, 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!

AlekseyTs Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 15, 2019 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Nov 15, 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!

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

Uh oh!

Uh oh!

AlekseyTs commented Nov 18, 2019

Uh oh!

333fred Nov 18, 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!

333fred Nov 18, 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!

333fred Nov 18, 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!

AlekseyTs Nov 18, 2019

Choose a reason for hiding this comment

Uh oh!

333fred Nov 18, 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!

AlekseyTs Nov 18, 2019

Choose a reason for hiding this comment

Uh oh!

333fred Nov 18, 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!

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

Uh oh!

333fred Nov 14, 2019 •

edited by AlekseyTs

Loading

gafter Nov 15, 2019 •

edited

Loading

gafter Nov 15, 2019 •

edited

Loading

gafter Nov 15, 2019 •

edited

Loading

AlekseyTs Nov 15, 2019 •

edited

Loading

gafter Nov 15, 2019 •

edited

Loading

gafter Nov 15, 2019 •

edited

Loading

AlekseyTs Nov 15, 2019 •

edited by 333fred

Loading

gafter Nov 15, 2019 •

edited

Loading

gafter commented Nov 15, 2019 •

edited

Loading

333fred Nov 18, 2019 •

edited by AlekseyTs

Loading

333fred Nov 18, 2019 •

edited

Loading

333fred Nov 18, 2019 •

edited

Loading

333fred Nov 18, 2019 •

edited

Loading

333fred Nov 18, 2019 •

edited

Loading

AlekseyTs Nov 18, 2019 •

edited

Loading

333fred commented Nov 18, 2019 •

edited by AlekseyTs

Loading