Move to the use site tracking for the purpose of recording used assemblies in VB by AlekseyTs · Pull Request #40640 · dotnet/roslyn

AlekseyTs · 2019-12-30T06:49:36Z

This change is continuing work started in #40560. It makes similar changes on VB side.

…blies in VB

gafter · 2019-12-31T21:25:21Z

src/Compilers/Core/Portable/Binding/BindingDiagnosticBag.cs

-        }
    }

+    // PROTOTYPE(UsedAssemblyReferences): Mesure performace impact of collecting more information during binding by using this class.


Mesure [](start = 42, length = 6)

misspelled. #Resolved

gafter · 2019-12-31T21:32:19Z

src/Compilers/Core/Portable/Collections/CollectionsExtensions.cs

+            return hashSet == null || hashSet.Count == 0;
+        }
+
+        internal static bool IsNullOrEmpty<T>([NotNullWhen(returnValue: false)] this IReadOnlyCollection<T>? hashSet)


hashSet [](start = 109, length = 7)

please select a more apropos name. #Resolved

gafter · 2019-12-31T21:32:24Z

src/Compilers/Core/Portable/Collections/CollectionsExtensions.cs

+{
+    internal static class CollectionsExtensions
+    {
+        internal static bool IsNullOrEmpty<T>([NotNullWhen(returnValue: false)] this ICollection<T>? hashSet)


hashSet [](start = 101, length = 7)

please select a more apropos name. #Resolved

gafter · 2019-12-31T21:37:14Z

src/Compilers/Test/Utilities/VisualBasic/Extensions.vb

+        End If
+    End Sub
+
+    <Extension()>


() [](start = 14, length = 2)

Please drop the () (or add them everywhere else).

Please drop the () (or add them everywhere else).

This doesn't violate any rules or guidelines.

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

Writing code in an internally inconsistent style is a bad programming practice. I am not reviewing only for things we have explicit rules for. I am reviewing for poor programming practices, among other things.

In any case, the "rule or guideline" violated here is in https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md which is referenced by https://github.com/dotnet/roslyn/blob/master/CONTRIBUTING.md and which states

When editing files, keep new code and changes consistent with the style in the files.

In reply to: 362283849 [](ancestors = 362283849,362283395)

gafter · 2019-12-31T22:02:27Z

...isualBasic/Portable/Lowering/AsyncRewriter/AsyncRewriter.AsyncMethodToClassRewriter.Await.vb

                        ' STMT:   this.builder.AwaitUnsafeOnCompleted(Of TAwaiter,TSM)((ByRef) $awaiterTemp, (ByRef) Me)
                        '  or
                        ' STMT:   this.builder.AwaitOnCompleted(Of TAwaiter,TSM)((ByRef) $awaiterTemp, (ByRef) Me)
+                        Dim useSiteInfo As CompoundUseSiteInfo(Of AssemblySymbol) = Nothing


Dim [](start = 24, length = 3)

Is this intended to be a bug fix? If so, do we have a test for it? If yes, please identify that test. #Closed

Is this intended to be a bug fix? If so, do we have a test for it? If yes, please identify that test.

This is intended as a change to follow a long standing requirement to report use-site errors for all operations/symbols that might have it. We don't have a test for this particular change and I do not believe it is worth spending time coming up with specific scenario demonstrating the problem. It is always safe to report use-site errors, at worse this can lead to a duplicate diagnostic being reported. If you prefer, I can undo the change and open an issue instead.

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

No, I was just wondering.

In reply to: 362286535 [](ancestors = 362286535,362285747)

gafter

Other than as noted (two parameters need renaming in CollectionsExtensions.cs, misspelling in core BindingDiagnosticBag.cs, and a style consistency issue in Extensions.vb), this looks good.

AlekseyTs · 2019-12-31T22:35:56Z

@dotnet/roslyn-compiler, @333fred Please review, need a second sign-off

AlekseyTs · 2020-01-02T17:34:35Z

@dotnet/roslyn-compiler, @333fred Please review, need a second sign-off

333fred · 2020-01-02T23:27:36Z

src/Compilers/Core/Portable/Binding/BindingDiagnosticBag.cs

-        }
    }

+    // PROTOTYPE(UsedAssemblyReferences): Mesure performace impact of collecting more information during binding by using this class.


performace [](start = 49, length = 10)

Also misspelled :) #Resolved

333fred · 2020-01-02T23:30:23Z

src/Compilers/Core/Portable/Binding/BindingDiagnosticBag.cs

+            }
+        }
+
+        internal void AddDependencies(IReadOnlyCollection<TAssemblySymbol>? dependencies)


Consider unifying this and the one above it with an AddDependencies(IEnumerable<TAssemblySymbol>? dependencies) #Closed

Actually, I suppose you can't because you need to know how many elements are in it. It's a shame that ICollection doesn't implement IReadonlyCollection.

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

333fred · 2020-01-02T23:35:31Z

src/Compilers/Core/Portable/Binding/UseSiteInfo.cs

+
+        public void MergeDependencies(ref TAssemblySymbol? primaryDependency, ref ImmutableHashSet<TAssemblySymbol>? secondaryDependencies)
+        {
+            secondaryDependencies = (secondaryDependencies ?? ImmutableHashSet<TAssemblySymbol>.Empty).Union(SecondaryDependencies ?? ImmutableHashSet<TAssemblySymbol>.Empty);


Is this the cause of the added Linq using above? #Resolved

If so, this could cause enumerator allocations as it won't use the struct-based enumerator on ImmutableHashSet

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

Is this the cause of the added Linq using above?

No, this Union method is defined on ImmutableHashSet. I assume it should be very efficient when both sides are ImmutableHashSets

In reply to: 362669358 [](ancestors = 362669358,362669134)

333fred · 2020-01-02T23:38:09Z

src/Compilers/Core/Portable/Binding/UseSiteInfo.cs

+
+            if (!object.Equals(primaryDependency, PrimaryDependency) && PrimaryDependency is object)
+            {
+                secondaryDependencies = secondaryDependencies.Add(PrimaryDependency);


The docs indicate this will be empty if DiagnosticInfo is an error. Should we be conditioning that here? #Resolved

The docs indicate this will be empty if DiagnosticInfo is an error. Should we be conditioning that here?

I do not think this needs an explicit check because PrimaryDependency would be null in that case.

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

333fred · 2020-01-02T23:43:13Z

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

            else
            {
-                Debug.Assert(info.DiagnosticInfo == null || addDependencies); // It would be strange to drop diagnostics, but record dependencies
+                Debug.Assert(info.DiagnosticInfo == null); // It would be strange to drop diagnostics, but record dependencies


// It would be strange to drop diagnostics, but record dependencies [](start = 59, length = 67)

Is this comment relevant anymore? #Resolved

Is this comment relevant anymore?

Yes.

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

333fred · 2020-01-03T00:28:20Z

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


 Namespace Microsoft.CodeAnalysis.VisualBasic

+    ' PROTOTYPE(UsedAssemblyReferences) Consider if it makes sense to move this type into its own file


Leaving off review (commit 1). Will pick back up tomorrow. #Closed

333fred

LGTM (commit 1)

AlekseyTs mentioned this pull request Dec 30, 2019

Move to the use site tracking for the purpose of recording used assemblies #40560

Merged

Move to the use site tracking for the purpose of recording used assem…

f4e6b3b

…blies in VB

AlekseyTs force-pushed the UsedAssemblies_09 branch from 86fc258 to f4e6b3b Compare December 31, 2019 20:29

AlekseyTs added the Area-Compilers label Dec 31, 2019

AlekseyTs marked this pull request as ready for review December 31, 2019 20:33

AlekseyTs requested a review from a team as a code owner December 31, 2019 20:33

AlekseyTs requested review from 333fred and gafter December 31, 2019 20:33

gafter reviewed Dec 31, 2019

View reviewed changes

gafter approved these changes Dec 31, 2019

View reviewed changes

333fred reviewed Jan 2, 2020

View reviewed changes

333fred reviewed Jan 3, 2020

View reviewed changes

333fred approved these changes Jan 3, 2020

View reviewed changes

PR feedback

bc412ee

AlekseyTs merged commit d6a5d04 into dotnet:features/UsedAssemblyReferences Jan 4, 2020


		Namespace Microsoft.CodeAnalysis.VisualBasic

		' PROTOTYPE(UsedAssemblyReferences) Consider if it makes sense to move this type into its own file

Conversation

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

Uh oh!

Uh oh!

gafter Dec 31, 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 Dec 31, 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 Dec 31, 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 Dec 31, 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!

gafter Dec 31, 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 Dec 31, 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!

Choose a reason for hiding this comment

Uh oh!

gafter left a comment • 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 Dec 31, 2019

Uh oh!

AlekseyTs commented Jan 2, 2020

Uh oh!

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

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

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

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

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

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

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

AlekseyTs commented Dec 30, 2019 •

edited

Loading

gafter Dec 31, 2019 •

edited by AlekseyTs

Loading

gafter Dec 31, 2019 •

edited by AlekseyTs

Loading

gafter Dec 31, 2019 •

edited by AlekseyTs

Loading

gafter Dec 31, 2019 •

edited

Loading

gafter Dec 31, 2019 •

edited

Loading

gafter Dec 31, 2019 •

edited

Loading

gafter left a comment •

edited

Loading

333fred Jan 2, 2020 •

edited by AlekseyTs

Loading

333fred Jan 2, 2020 •

edited

Loading

333fred Jan 2, 2020 •

edited by AlekseyTs

Loading

333fred Jan 2, 2020 •

edited by AlekseyTs

Loading

333fred Jan 2, 2020 •

edited by AlekseyTs

Loading

333fred Jan 3, 2020 •

edited

Loading