Align the implementations of IAssemblySymbol.GivesAccessTo by gafter · Pull Request #26727 · dotnet/roslyn

gafter · 2018-05-08T21:40:17Z

It now has a shared core implementation that permits operation across IAssemblySymbol implementations.
Fixes #26459
Fixes #26723
One remaining difference is in the treatment of a strong named assembly and a weak named assembly. C# treats them as being (capable of being) related for error recovery. VB does not. We would like to strengthen C# but that may be a breaking change, so we're leaving it as is, at least for now.
See also #26722

It now has a shared core implementation that permits operation across IAssemblySymbol implementations. Fixes dotnet#26459 Fixes dotnet#26723 One remaining difference is in the treatment of a strong named assembly and a weak named assembly. C# treats them as being (capable of being) related for error recovery. VB does not. We would like to strengthen C# but that may be a breaking change, so we're leaving it as is, at least for now. See also dotnet#26722

gafter · 2018-05-08T21:55:24Z

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

-        Protected Function PerformIVTCheck(key As ImmutableArray(Of Byte), otherIdentity As AssemblyIdentity) As IVTConclusion
-            ' Implementation of this function in C# compiler is somewhat different, but we believe
-            ' that the difference doesn't affect any real world scenarios that we know/care about.
-            ' At the moment we don't feel it is worth porting the logic, but we might reconsider in the future. 


That moment has passed. But rather than porting the logic we moved it to a shared layer. #ByDesign

gafter · 2018-05-08T21:58:27Z

@dotnet/roslyn-compiler Please review this refactoring of duplicate compiler code to shared code.
@AlekseyTs I suspect you are interested in this.
#Resolved

jcouv · 2018-05-09T18:02:10Z

src/Compilers/Core/Portable/IVTConclusion.cs

+        /// </summary>
+        NoRelationshipClaimed
+    }
+}


Not related to this PR, but I thought I should mention it since you're working in that area: #17322 (improving diagnostics related to IVT) #Resolved

Yes, I expect that and #26722 to be addressed at the same time.

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

jcouv · 2018-05-09T18:03:48Z

src/Compilers/Core/Portable/Symbols/ISymbolExtensions_PerformIVTCheck.cs

+using System.Collections.Immutable;
+using Microsoft.CodeAnalysis.Collections;
+
+namespace Microsoft.CodeAnalysis


Nit: Should the filename use a dot as a separator instead of underscore, to be consistent with rest of codebase?
#ByDesign

The convention I've seen is to use dot as a separator for a nested type and underscore as a separator for a part of a partial type.

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

jcouv · 2018-05-09T18:10:38Z

src/Compilers/Core/Portable/Symbols/ISymbolExtensions_PerformIVTCheck.cs

+            // assembly is "Jones.DLL", and that Smith has named Jones as a friend (that is a precondition
+            // to calling this method). Whether we allow Jones to see internals of Smith depends on these four factors:
+            //
+            // q1) Is Smith strong-named?


Nit: Could I suggest "Granting.dll" and "Wanting.dll" to align on terminology? (no "this"/"other", "Smith"/"Jones")
I recognize this is just moved code/comment though... #Resolved

I tried doing that and now the comment is readable!

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

jcouv · 2018-05-09T18:29:07Z

src/Compilers/Core/Portable/Symbols/ISymbolExtensions_PerformIVTCheck.cs

+            //   to strong-named Jones, which then references Smith. However, we still want to give friend
+            //   access to Jones for the purposes of semantic analysis.
+            //
+            // TODO: Roslyn C# does not yet give an error in other circumstances whereby a strong-named assembly


TODO's should be replaced with open issues. #Resolved

jcouv · 2018-05-09T18:54:23Z

src/Compilers/CSharp/Test/Emit/Attributes/InternalsVisibleToAndStrongNameTests.cs

+            // We allow John to access Paul's internal Goo even though strong-named John should not be referencing weak-named Paul.
+            // Paul has, after all, specifically granted access to John.
+
+            // TODO: During emit time we should produce an error that says that a strong-named assembly cannot reference


TODO [](start = 15, length = 4)

Nit: TODO #Resolved

jcouv · 2018-05-09T18:55:11Z

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

+                result = potentialGiverOfAccess.Identity.PerformIVTCheck(Me.PublicKey, key)
+
+                If result = IVTConclusion.Match Then
+                    ' Note that C# includes  OrElse result = IVTConclusion.OneSignedOneNot


[](start = 43, length = 2)

Nit: double space #ByDesign

The double space is intentional, as it precedes a code quotation. #ByDesign

jcouv

LGTM Thanks

OmarTawfik · 2018-05-09T21:11:09Z

src/Compilers/Core/CodeAnalysisTest/GivesAccessTo.cs

+            Assert.True(VB.ContainingAssembly.GivesAccessTo(CS.ContainingAssembly));
+            Assert.False(VB.ContainingAssembly.GivesAccessTo(CS2.ContainingAssembly));
+
+        }


nit: empty line #Resolved

OmarTawfik · 2018-05-09T21:20:15Z

src/Compilers/Core/Portable/Symbols/ISymbolExtensions.cs

        }
+
+        internal static bool IsNetModule(this IAssemblySymbol assembly) =>
+            assembly is ISourceAssemblySymbol sourceAssembly && sourceAssembly.Compilation.Options.OutputKind == OutputKind.NetModule;


Compilation.Options.OutputKind == OutputKind.NetModule [](start = 79, length = 54)

perhaps use the OutputKind.IsNetModule() helper? #Resolved

OmarTawfik

* dotnet/master: (732 commits) Disable procdump on Spanish runs use named args. Exclude System.Diagnostics.DiagnosticSource from PortableFacades Exclude System.Net.Http from PortableFacades Align the implementations of IAssemblySymbol.GivesAccessTo (#26727) Fixed exception message in GetSemanticModel (#26659) Implement IDiscardSymbol.Equals and GetHashCode (#26720) Produce correct diagnostics on implicit operator conversions for stackalloc expressions (#26248) Remove special case handling of CreateDiagnosticProviderAndFixer throwing an exception Add verbose logging to DevDivInsertionFiles if you want it Exclude Microsoft.Composition as something we build a dependency map for Fix the message for Make Field Readonly Update LangVersion to 7.3 (#26698) fix extension deployment and add explainatory comment Add TaskExtensions to compiler package Add System.Diagnostics.DiagnosticSource to SignToolData.json Include missing facade: System.Diagnostics.DiagnosticSource Use default expressions. Remove forwarding method. Revert project chnage. ...

gafter added the Area-Compilers label May 8, 2018

gafter added this to the 15.8 milestone May 8, 2018

gafter self-assigned this May 8, 2018

gafter requested review from a team as code owners May 8, 2018 21:40

Remove an accidentally-included method.

6ccd1ad

gafter added the Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality label May 8, 2018

gafter commented May 8, 2018

View reviewed changes

gafter mentioned this pull request May 8, 2018

Align the implementations of IAssemblySymbol.GivesAccessTo #26469

Closed

jcouv reviewed May 9, 2018

View reviewed changes

jcouv approved these changes May 9, 2018

View reviewed changes

Clean up comments.

e10ef61

OmarTawfik reviewed May 9, 2018

View reviewed changes

OmarTawfik approved these changes May 9, 2018

View reviewed changes

Tiny changes recommended in review.

4d78248

gafter merged commit cb21f0e into dotnet:master May 10, 2018

Conversation

gafter commented May 8, 2018

Uh oh!

gafter May 8, 2018 • 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 May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv May 9, 2018 • edited by gafter 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 May 9, 2018

Choose a reason for hiding this comment

Uh oh!

jcouv May 9, 2018 • edited by gafter 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 May 9, 2018

Choose a reason for hiding this comment

Uh oh!

jcouv May 9, 2018 • edited by gafter 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 May 9, 2018

Choose a reason for hiding this comment

Uh oh!

jcouv May 9, 2018 • edited by gafter 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 May 9, 2018 • edited by gafter 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 May 9, 2018 • edited by gafter 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 May 9, 2018 • 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 left a comment

Choose a reason for hiding this comment

Uh oh!

OmarTawfik May 9, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OmarTawfik May 9, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OmarTawfik left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gafter May 8, 2018 •

edited

Loading

gafter commented May 8, 2018 •

edited

Loading

jcouv May 9, 2018 •

edited by gafter

Loading

jcouv May 9, 2018 •

edited by gafter

Loading

jcouv May 9, 2018 •

edited by gafter

Loading

jcouv May 9, 2018 •

edited by gafter

Loading

jcouv May 9, 2018 •

edited by gafter

Loading

jcouv May 9, 2018 •

edited by gafter

Loading

gafter May 9, 2018 •

edited

Loading

OmarTawfik May 9, 2018 •

edited by gafter

Loading

OmarTawfik May 9, 2018 •

edited by gafter

Loading