Implement GetUsedAssemblyReferences API for VB by AlekseyTs · Pull Request #40987 · dotnet/roslyn

AlekseyTs · 2020-01-15T19:20:22Z

No description provided.

AlekseyTs · 2020-01-15T22:15:32Z

@dotnet/roslyn-compiler Please review #Closed

AlekseyTs · 2020-01-16T15:07:50Z

@dotnet/roslyn-compiler Please review #Closed

AlekseyTs · 2020-01-16T20:06:52Z

@dotnet/roslyn-compiler Please review #Closed

jcouv · 2020-01-17T00:30:37Z

Looking #Closed

jcouv · 2020-01-17T00:31:52Z

eng/build.ps1

  [switch][Alias('test')]$testDesktop,
  [switch]$testCoreClr,
  [switch]$testIOperation,
+  [switch]$testUsedAssemblies,


$testUsedAssemblies [](start = 10, length = 19)

Should we pass this flag in one of the CI legs, or is this intended for local testing only at this point? #Resolved

Should we pass this flag in one of the CI legs, or is this intended for local testing only at this point?

Once the feature is moved to master, I think.

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

jcouv · 2020-01-17T00:33:56Z

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

        }

-        [Fact(Skip = "PROTOTYPE(UsedAssemblyReferences): The test hook is blocked by https://github.com/dotnet/roslyn/issues/39970")]
+        [ConditionalFact(typeof(NoUsedAssembliesValidation))] // The test hook is blocked by https://github.com/dotnet/roslyn/issues/39970


// The test hook is blocked by #39970 [](start = 62, length = 76)

The linked issue was closed already (fixed in master) #Closed

jcouv · 2020-01-17T01:31:21Z

Done with C# files. Will look at the rest tomorrow. #Closed

jcouv · 2020-01-17T21:06:52Z

src/Compilers/VisualBasic/Portable/Symbols/UsedAssemblies.vb

+        End Function
+
+
+        Private Sub AddUsedAssembly(dependency As AssemblySymbol, stack As ArrayBuilder(Of AssemblySymbol))


Private [](start = 8, length = 7)

nit: formatting above #Resolved

jcouv · 2020-01-17T21:27:15Z

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

        ' It normally happens during actual compile, but also happens when getting emit diagnostics for 
        ' testing purposes.
        Private ReadOnly _doEmitPhase As Boolean
+        Private ReadOnly _doLoweringPhase As Boolean


_doLoweringPhase [](start = 25, length = 16)

nit: I'd consider leaving a comment why/when this is useful. To get used assemblies we need to run lowering phase, but for regular diagnostic information we don't. #Resolved

jcouv · 2020-01-17T21:32:06Z

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

                            isBodySynthesized:=True)

-                        If Not diagnosticsThisMethod.HasAnyErrors Then
+                        If Not diagnosticsThisMethod.HasAnyErrors AndAlso DoEmitPhase Then


DoEmitPhase [](start = 74, length = 11)

nit: consider swapping checks (I assume diagnosticsThisMethod.HasAnyErrors is more expensive). Possibly also below #Resolved

jcouv · 2020-01-17T21:40:49Z

src/Compilers/VisualBasic/Portable/Symbols/Source/SourceFile.vb

                                           diagnostics As BindingDiagnosticBag)
            ' TODO: Dev10 reports error on specific type parts rather than the import
            ' (reporting error on Object rather than C in C = A(Of Object) for instance).
+            Dim clauseDiagnostics = BindingDiagnosticBag.GetInstance()


clauseDiagnostics [](start = 16, length = 17)

nit: Is there a gain from re-using instance this way, rather than simply getting a new instance on every iteration (from within ValidateImportsClause)? May be marginally simpler to read.
I was thrown off initially, wondering what this bag was for and why it was discarded... #WontFix

Is there a gain from re-using instance this way, rather than simply getting a new instance on every iteration

Avoiding allocating within a loop on every iteration is always a good thing.

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

jcouv · 2020-01-17T21:43:11Z

src/Compilers/VisualBasic/Portable/Binding/Binder_Imports.vb

+                Dim diagBag = New BindingDiagnosticBag(diagnostics, dependenciesBag)
+#Else
+                Dim diagBag = New BindingDiagnosticBag(diagnostics)
+#End If


nit: double empty line below #Resolved

jcouv · 2020-01-17T21:43:58Z

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

+            AssertUsedAssemblyReferences(comp, expected, new DiagnosticDescription[] { }, new DiagnosticDescription[] { }, specificReferencesToAssert);
        }

        private Compilation AssertUsedAssemblyReferences(string source, MetadataReference[] references, params MetadataReference[] expected)


AssertUsedAssemblyReferences [](start = 28, length = 28)

nit: it's odd to have an Assert method return a result. Consider renaming. Also, I was generally a bit confused about the many overloads, especially with use of params. It's probably okay within IDE, but harder as text review. #WontFix

jcouv

LGTM Thanks (iteration 1). I did only spot checking on the massive test file.

AlekseyTs · 2020-01-17T21:46:30Z

@dotnet/roslyn-compiler Please review, need a second sign-off #Closed

AlekseyTs · 2020-01-21T18:07:07Z

@dotnet/roslyn-compiler Please review, need a second sign-off #Closed

333fred · 2020-01-21T20:34:55Z

eng/build.ps1

  }

+  if ($testUsedAssemblies) {
+    $env:ROSLYN_TEST_USEDASSEMBLIES = "true"


Should we consider adding a new CI leg to run this check? /cc @jaredpar. #Resolved

Should we consider adding a new CI leg to run this check?

I think we should do this once the feature is moved to master.

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

333fred · 2020-01-21T21:10:54Z

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

-                    _diagnostics.AddRange(diagnosticsThisMethod)
-                    diagnosticsThisMethod.Free()
-                Else
+                If Not diagnosticsThisMethod.HasAnyErrors AndAlso DoEmitPhase Then


Not sure if you did the swap up here as well, just making sure that Julien's below comment is seen here too.

nit: consider swapping checks (I assume diagnosticsThisMethod.HasAnyErrors is more expensive). Possibly also below #Resolved

src/Compilers/VisualBasic/Portable/Symbols/UsedAssemblies.vb

333fred · 2020-01-21T21:51:41Z

src/Compilers/VisualBasic/Portable/Symbols/UsedAssemblies.vb

+                ' This, for example, takes care of including facade assemblies that forward types around.
+                If _lazyUsedAssemblyReferences IsNot Nothing Then
+                    SyncLock _lazyUsedAssemblyReferences
+                        If _usedAssemblyReferencesFrozen OrElse Volatile.Read(_usedAssemblyReferencesFrozen) Then


_usedAssemblyReferencesFrozen [](start = 27, length = 29)

Can we do a read of this outside the lock? #Resolved

Can we do a read of this outside the lock?

We do this at the beginning of the function and I think it make sense to check after the lock is acquired too in order to avoid doing the work if it was done while we were waiting for the lock.

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

333fred · 2020-01-21T22:00:48Z

src/Compilers/VisualBasic/Portable/Symbols/UsedAssemblies.vb

+#If DEBUG Then
+            Dim wasFrozen As Boolean = _usedAssemblyReferencesFrozen
+#End If
+            Dim result As Boolean = _lazyUsedAssemblyReferences.Add(assembly)


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

Nit: a better name would be added, would make the assertion below easier to read at a glance. #Resolved

333fred · 2020-01-21T22:04:36Z

src/Compilers/VisualBasic/Portable/Symbols/Source/SourceFile.vb

+        )
+            Dim type = TryCast(namespaceOrType, TypeSymbol)
+            If type IsNot Nothing Then
+                clauseDiagnostics.Clear()


I think it would be better to clear these on every loop in the calling method, rather than splitting the semantics of how this particular BindingDiagnosticsBag is supposed to be used across multiple methods. #Resolved

I think it would be better to clear these on every loop in the calling method, rather than splitting the semantics of how this particular BindingDiagnosticsBag is supposed to be used across multiple methods.

I don't think I agree. The caller is only responsible for providing an instance, it doesn't use this bag at all. Usage in this method depends on starting with an empty bag, therefore, I believe it is more appropriate to do this here, rather than adding an extra contract for the caller. That, in fact would split usage semantics across multiple methods, I think.

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

333fred · 2020-01-21T22:23:10Z

interface I1

Nit: interface -> Interface, and more in this file. #WontFix

Refers to: src/Compilers/VisualBasic/Test/Symbol/UsedAssembliesTests.vb:21 in 4609ed6. [](commit_id = 4609ed6, deletion_comment = False)

333fred · 2020-01-21T22:27:41Z

interface I1

And a bunch more lowercase things (class, public, and a few others) that I've noticed so far.

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

Refers to: src/Compilers/VisualBasic/Test/Symbol/UsedAssembliesTests.vb:21 in 4609ed6. [](commit_id = 4609ed6, deletion_comment = False)

333fred

LGTM (commit 1). Feedback is fine in a followup where appropriate.

AlekseyTs · 2020-01-22T00:04:04Z

interface I1

Nit: interface -> Interface, and more in this file.

This is inside string literals.

In reply to: 576915150 [](ancestors = 576915150,576913297)

Refers to: src/Compilers/VisualBasic/Test/Symbol/UsedAssembliesTests.vb:21 in 4609ed6. [](commit_id = 4609ed6, deletion_comment = False)

Implement GetUsedAssemblyReferences API for VB

4609ed6

AlekseyTs added the Area-Compilers label Jan 15, 2020

AlekseyTs requested review from a team as code owners January 15, 2020 19:20

jcouv self-assigned this Jan 17, 2020

jcouv reviewed Jan 17, 2020

View reviewed changes

jcouv approved these changes Jan 17, 2020

View reviewed changes

333fred reviewed Jan 21, 2020

View reviewed changes

src/Compilers/VisualBasic/Portable/Symbols/UsedAssemblies.vb Show resolved Hide resolved

333fred reviewed Jan 21, 2020

View reviewed changes

333fred approved these changes Jan 21, 2020

View reviewed changes

PR feedback

7c1f1d4

AlekseyTs merged commit f5b4ff0 into dotnet:features/UsedAssemblyReferences Jan 22, 2020

		End Function


		Private Sub AddUsedAssembly(dependency As AssemblySymbol, stack As ArrayBuilder(Of AssemblySymbol))

Conversation

AlekseyTs commented Jan 15, 2020

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

jcouv commented Jan 17, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Jan 17, 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!

AlekseyTs Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 17, 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!

jcouv commented Jan 17, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Jan 17, 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!

jcouv Jan 17, 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!

jcouv Jan 17, 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!

jcouv Jan 17, 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!

AlekseyTs Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 17, 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!

jcouv Jan 17, 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!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

333fred Jan 21, 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!

AlekseyTs Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

333fred Jan 21, 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!

Uh oh!

333fred Jan 21, 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

AlekseyTs commented Jan 15, 2020 •

edited

Loading

AlekseyTs commented Jan 16, 2020 •

edited

Loading

AlekseyTs commented Jan 16, 2020 •

edited

Loading

jcouv commented Jan 17, 2020 •

edited by AlekseyTs

Loading

jcouv Jan 17, 2020 •

edited by AlekseyTs

Loading

jcouv Jan 17, 2020 •

edited

Loading

jcouv commented Jan 17, 2020 •

edited by AlekseyTs

Loading

jcouv Jan 17, 2020 •

edited by AlekseyTs

Loading

jcouv Jan 17, 2020 •

edited by AlekseyTs

Loading

jcouv Jan 17, 2020 •

edited by AlekseyTs

Loading

jcouv Jan 17, 2020 •

edited by AlekseyTs

Loading

jcouv Jan 17, 2020 •

edited by AlekseyTs

Loading

jcouv Jan 17, 2020 •

edited by AlekseyTs

Loading

AlekseyTs commented Jan 17, 2020 •

edited

Loading

AlekseyTs commented Jan 21, 2020 •

edited

Loading

333fred Jan 21, 2020 •

edited by AlekseyTs

Loading

333fred Jan 21, 2020 •

edited by AlekseyTs

Loading

333fred Jan 21, 2020 •

edited by AlekseyTs

Loading

333fred Jan 21, 2020 •

edited by AlekseyTs

Loading

333fred Jan 21, 2020 •

edited by AlekseyTs

Loading

333fred commented Jan 21, 2020 •

edited by AlekseyTs

Loading