Diagnostic Improvements for error #0281 #27052

fayrose · 2018-05-22T19:50:59Z

Customer scenario

What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)

When one tries to access a public method in an internal class, or when one tries to access an internal method in a public class.

Bugs this fixes

#17322

Workarounds, if any

No, diagnostic improvement.

Risk

Very low.

Performance impact

Negligible: differentiates between two error cases but does not impact performance.

Is this a regression from a previous update?

Yes, regression from native compiler.

Root cause analysis

How did we miss it? What tests are we adding to guard against it in the future?
Insufficient test coverage.

How was the bug found?

(E.g. customer reported it vs. ad hoc testing)

Marek Safar - external-ish.

Test documentation updated?

No, diagnostic improvement.

If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.

agocke · 2018-05-22T20:36:18Z

Looks like the bootstrap is failing. Build.cmd -bootstrap should replicate the failure.

jaredpar · 2018-05-23T00:37:52Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceAssemblySymbol.cs

@@ -1909,7 +1909,7 @@ private void CheckOptimisticIVTAccessGrants(DiagnosticBag bag)

                    if (conclusion == IVTConclusion.PublicKeyDoesntMatch)
                        bag.Add(ErrorCode.ERR_FriendRefNotEqualToThis, NoLocation.Singleton,
-                                                                      otherAssembly.Identity);
+                                                                      otherAssembly.Identity.ToString(), this.ContainingAssembly.Identity.ToString());


I don't believe we should be calling otherAssembly.Identity.ToString here. That will cause it to be captured under the current UI culture. Diagnostics can have their messages realized under a different culture.

toString() has been overridden to be culture-independent.

Why use ToString() here rather than letting the MessageProvider serialize the arguments?

I usually prefer to be explicit on how things get serialized to strings -- Jared's comment reflects that this is usually a very deliberate decision and being explicit signals intent better, IMHO.

jaredpar · 2018-05-23T00:38:40Z

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

@@ -63,6 +64,23 @@ public static ImmutableArray<T> AsImmutableOrNull<T>(this IEnumerable<T> items)
            return ImmutableArray.CreateRange<T>(items);
        }

+        public static string PublicKeyToString(this ImmutableArray<byte> key)


Probably don't want this as a general purpose extension method on ImmutableArray<byte>. Feels pretty specific to a scenario.

CyrusNajmabadi · 2018-05-23T00:39:11Z

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

+
+                 foreach (ImmutableArray<byte> key in unwrappedSymbol.ContainingAssembly.GetInternalsVisibleToPublicKeys(this.ContainingType.ContainingAssembly.Name))
+                 {
+                    friendRefNotEqualToThis = key.SequenceEqual(this.ContainingType.ContainingAssembly.Identity.PublicKey) ? false : friendRefNotEqualToThis;


indentation seems off here...

cston · 2018-05-23T19:12:22Z

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

+                 {
+                     var unwrappedSymbols = ImmutableArray.Create<Symbol>(unwrappedSymbol);
+                     diagInfo = diagnose ? new CSDiagnosticInfo(ErrorCode.ERR_BadAccess, new[] { unwrappedSymbol }, unwrappedSymbols, additionalLocations: ImmutableArray<Location>.Empty) : null;
+                 }


Consider checking diagnose separately from each individual case. And perhaps combine all checks with ?::

var diagInfo = diagnose ? inaccessibleViaQualifier ? new CSDiagnosticInfo(...) : friendNotEqualToThis ? new CSDiagnosticInfo(...) : new CSDiagnosticInfo(...) : null;

cston · 2018-05-23T19:16:54Z

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

+                 foreach (ImmutableArray<byte> key in unwrappedSymbol.ContainingAssembly.GetInternalsVisibleToPublicKeys(this.ContainingType.ContainingAssembly.Name))
+                 {
+                     friendRefNotEqualToThis = key.SequenceEqual(this.ContainingType.ContainingAssembly.Identity.PublicKey) ? false : friendRefNotEqualToThis;
+                 }


Can be combined with friendRefNotEqualToThis assignment above:

bool friendRefNotEqualToThis = unwrappedSymbol != null && ... && unwrappedSymbol.ContainingAssembly.GetInternalsVisibleToPublicKeys( this.ContainingAssembly.Name).Any((key, publicKey) => key.SequenceEqual(publicKey), this.ContainingAssembly.Identity.PublicKey);

That would capture this, and capturing lambdas are prohibited in compiler code.

My mistake, there is no Any overload that takes an additional arg parameter passed to the predicate. @agocke is correct that means the lambda would need to capture a local or this.

…nder lookup

agocke · 2018-05-23T19:50:52Z

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

@@ -1148,21 +1147,33 @@ internal SingleLookupResult CheckViability(Symbol symbol, int arity, LookupOptio
                return LookupResult.Inaccessible(symbol, diagInfo);
            }
            else if (!InCref &&
-                     !this.IsAccessible(unwrappedSymbol,


Looks like the old whitespace was correct

agocke · 2018-05-23T19:52:36Z

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

-                    var unwrappedSymbols = ImmutableArray.Create<Symbol>(unwrappedSymbol);
-                    diagInfo = diagnose ? new CSDiagnosticInfo(ErrorCode.ERR_BadAccess, new[] { unwrappedSymbol }, unwrappedSymbols, additionalLocations: ImmutableArray<Location>.Empty) : null;
-                }
+                 bool friendRefNotEqualToThis = unwrappedSymbol != null && unwrappedSymbol.ContainingAssembly.GetInternalsVisibleToPublicKeys(this.ContainingType.ContainingAssembly.Name).Any()


I don't think unwrappedSymbol can be null here. It looks like it's always assigned above.

agocke · 2018-05-23T19:53:43Z

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

+
+                 foreach (ImmutableArray<byte> key in unwrappedSymbol.ContainingAssembly.GetInternalsVisibleToPublicKeys(this.ContainingType.ContainingAssembly.Name))
+                 {
+                     friendRefNotEqualToThis = key.SequenceEqual(this.ContainingType.ContainingAssembly.Identity.PublicKey) ? false : friendRefNotEqualToThis;


I would move all this logic into a local function. That way if the first if statement is true, we don't waste any time doing unnecessary calculations.

agocke · 2018-05-23T19:54:06Z

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

@@ -1196,7 +1207,7 @@ internal SingleLookupResult CheckViability(Symbol symbol, int arity, LookupOptio
                return LookupResult.Good(symbol);
            }
        }
-
+ 


Accidental whitespace change.

agocke · 2018-05-23T19:55:05Z

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

-                }
-
+                 bool friendRefNotEqualToThis = unwrappedSymbol != null && unwrappedSymbol.ContainingAssembly.GetInternalsVisibleToPublicKeys(this.ContainingType.ContainingAssembly.Name).Any()
+                            && unwrappedSymbol.DeclaredAccessibility.Equals(Accessibility.Internal);


You should be able to use == instead of .Equals here.

agocke · 2018-05-23T19:56:37Z

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

-                    diagInfo = diagnose ? new CSDiagnosticInfo(ErrorCode.ERR_BadAccess, new[] { unwrappedSymbol }, unwrappedSymbols, additionalLocations: ImmutableArray<Location>.Empty) : null;
-                }
-
+                 bool friendRefNotEqualToThis = unwrappedSymbol != null && unwrappedSymbol.ContainingAssembly.GetInternalsVisibleToPublicKeys(this.ContainingType.ContainingAssembly.Name).Any()


Can ContainingAssembly.Name be null here?

agocke · 2018-05-23T19:58:30Z

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

+                     friendRefNotEqualToThis = key.SequenceEqual(this.ContainingType.ContainingAssembly.Identity.PublicKey) ? false : friendRefNotEqualToThis;
+                 }
+
+                 diagInfo = diagnose ?


I'm not sure this is what @cston had in mind. I think he just wanted the diagnose check lifted to before the first if statement, but I may be wrong. complete conversion to ? : doesn't look more readable to me.

333fred · 2018-05-23T20:20:26Z

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

@@ -7,6 +7,7 @@
 using System.IO;
 using System.Linq;
 using System.Runtime.CompilerServices;
+using System.Text;


Please revert the change to this file, import doesn't appear to be necessary.

jaredpar · 2018-05-23T20:32:13Z

src/Compilers/Core/Portable/MetadataReference/AssemblyIdentity.DisplayName.cs

@@ -58,6 +58,20 @@ public override string ToString()
            return GetDisplayName(fullKey: false);
        }

+
+        internal static string PublicKeyToString(ImmutableArray<byte> key)


Nit: double blank line above. #Resolved

agocke · 2018-05-23T20:43:42Z

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

-                    var unwrappedSymbols = ImmutableArray.Create<Symbol>(unwrappedSymbol);
-                    diagInfo = diagnose ? new CSDiagnosticInfo(ErrorCode.ERR_BadAccess, new[] { unwrappedSymbol }, unwrappedSymbols, additionalLocations: ImmutableArray<Location>.Empty) : null;
-                }
+                 bool friendRefNotEqualToThis = getFriendRefNotEqualToThis();


Calling it up front means we still pay for the execution. I would inline this call.

agocke · 2018-05-23T20:44:25Z

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


+            bool getFriendRefNotEqualToThis()
+            {
+                if (inaccessibleViaQualifier)


I think this check should be outside of the local funciton.

cston · 2018-05-24T05:57:09Z

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

+                {
+                    Debugger.Launch();
+                    return false;
+                }


Is this catch block necessary?

cston · 2018-05-24T05:57:36Z

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

+            {
+                try
+                {
+                    if (inaccessibleViaQualifier)


The caller already checks this.

cston · 2018-05-24T06:10:15Z

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

+                            && unwrappedSymbol.DeclaredAccessibility == Accessibility.Internal
+                            && this.Compilation.Assembly.PublicKey != null;
+
+                    if (temp && unwrappedSymbol.ContainingAssembly.GetInternalsVisibleToPublicKeys(this.Compilation.AssemblyName).Any())


Is the && unwrappedSymbol...Any() check needed?

cston · 2018-05-24T06:11:41Z

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

+                    {
+                        foreach (ImmutableArray<byte> key in unwrappedSymbol.ContainingAssembly.GetInternalsVisibleToPublicKeys(this.Compilation.AssemblyName))
+                        {
+                            temp = key.SequenceEqual(this.Compilation.Assembly.Identity.PublicKey) ? false : temp;


No need to check keys beyond the first match.

if (key.SequenceEqual(this.Compilation.Assembly.Identity.PublicKey)) { return false; }

cston · 2018-05-24T06:19:53Z

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

+            var friendClass = CreateCompilation(@"
+using System.Runtime.CompilerServices;
+[ assembly: InternalsVisibleTo(""cs0281, PublicKey=00240000048000009400000006020000002400005253413100040000010001002b986f6b5ea5717d35c72d38561f413e267029efa9b5f107b9331d83df657381325b3a67b75812f63a9436ceccb49494de8f574f8e639d4d26c0fcf8b0e9a1a196b80b6f6ed053628d10d027e032df2ed1d60835e5f47d32c9ef6da10d0366a319573362c821b5f8fa5abc5bb22241de6f666a85d82d6ba8c3090d01636bd2bb"") ]
+public class FriendClass


Should the class be named PublicClass?

cston · 2018-05-24T06:27:15Z

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

+                        Diagnostic((ErrorCode)errorCode, "MyMethod").WithArguments("FriendClass.MyMethod()").WithLocation(7, 15)
+                    );
+                    break;
+                default: throw new NotImplementedException();


The InlineData makes this test difficult to read. Could this be written more explicitly and without InlineData? Perhaps:

var source1 = @" using System.Runtime.CompilerServices; [assembly: InternalsVisibleTo(...)] public class PublicClass { internal static void InternalMethod() { } protected static void ProtectedMethod() { } private static void PrivateMethod() { } internal protected static void InternalProtectedMethod() { } private protected static void PrivateProtectedMethod() { } }"; var source2 = @" class Test { static void Main() { PublicClass.InternalMethod(); PublicClass.ProtectedMethod(); ... } }";

agocke · 2018-05-24T20:55:09Z

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

+                    {
+                       diagInfo = new CSDiagnosticInfo(ErrorCode.ERR_BadProtectedAccess, unwrappedSymbol, accessThroughType, this.ContainingType);
+                    }
+                    else if (getFriendRefNotEqualToThis())


We usually prefix bool-returning functions and properties with words like Is or Has. I would call this function something like isBadIvtSpecification.

agocke · 2018-05-24T20:58:25Z

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

+                       // 		PublicClass.InternalMethod();
+                      Diagnostic(ErrorCode.ERR_FriendRefNotEqualToThis, "InternalMethod").WithArguments("Paul, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null", "").WithLocation(7, 15),
+                       // (8,21): error CS0281: Friend access was granted by 'Paul, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null', but the public key of the output assembly ('') does not match that specified by the InternalsVisibleTo attribute in the granting assembly.
+                       //         PublicClass.ProtectedMethod();


This doesn't look right. protected members are not affected by InternalsVisibleTo, so we should not provide the new error message for protected accesses, only things that have effectively "internal" access.

agocke · 2018-05-24T21:01:47Z

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

+            );
+
+        }
+


Since we previously hit a bug due to accessing the symbol from a using alias, I would expect a regression test that accesses an internal symbol via a using alias, like

using MyAlias = OtherNamespace.FriendClass;

agocke · 2018-05-29T07:43:20Z

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

+                      Diagnostic(ErrorCode.ERR_NoSuchMember, "PrivateMethod").WithArguments("PublicClass", "PrivateMethod").WithLocation(9, 21),
+                      // (10,21): error CS0122: 'PublicClass.InternalProtectedMethod()' is inaccessible due to its protection level
+                      //         PublicClass.InternalProtectedMethod();
+                      Diagnostic(ErrorCode.ERR_BadAccess, "InternalProtectedMethod").WithArguments("PublicClass.InternalProtectedMethod()").WithLocation(10, 21),


I think these two are the wrong message. "protected internal" and "private protected" should be the same as "internal"

agocke

LGTM assuming tests pass and minor comments are addressed

agocke · 2018-05-30T17:54:03Z

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


+            bool IsBadIvtSpecification()
+            {
+                if ((unwrappedSymbol.DeclaredAccessibility == Accessibility.Internal || unwrappedSymbol.DeclaredAccessibility == Accessibility.ProtectedAndInternal 


I would split these each onto their own line for readability, i.e.

(unwrappedSymbol.DeclaredAccessibility == Accessibility.Internal || unwrappedSymbol.DeclaredAccessibility == Accessibility.ProtectedAndInternal || unwrappedSymbol.DeclaredAccessibility == Accessibility.ProtectedOrInternal)

agocke · 2018-05-30T18:23:36Z

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

+                if ((unwrappedSymbol.DeclaredAccessibility == Accessibility.Internal || unwrappedSymbol.DeclaredAccessibility == Accessibility.ProtectedAndInternal 
+                    || unwrappedSymbol.DeclaredAccessibility == Accessibility.ProtectedOrInternal)
+                // Ensures that during binding we don't ask for public key which results in attribute binding and stack overflow.
+                // If looking up attributes, don't ask for public key.


I think these comment lines are out of order (it reads better if the lower one is at the top).

agocke · 2018-05-30T18:25:15Z

Looks like there are a few new broken tests that just need rebaselining.

jaredpar · 2018-05-31T16:56:25Z

retest this please

mmitche · 2018-05-31T17:26:45Z

@dotnet-bot test windows_debug_spanish_unit32_prtest

jaredpar · 2018-05-31T21:17:16Z

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

-                return LookupResult.Inaccessible(symbol, diagInfo);
+                 if (!diagnose)
+                 {
+                    diagInfo = null;


Indent still seems off here. Currently three spaces, should be four.

jaredpar · 2018-05-31T21:17:43Z

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

+                 }
+                 else if (IsBadIvtSpecification())
+                 {
+                        diagInfo = new CSDiagnosticInfo(ErrorCode.ERR_FriendRefNotEqualToThis, unwrappedSymbol.ContainingAssembly.Identity.ToString(), AssemblyIdentity.PublicKeyToString(this.Compilation.Assembly.PublicKey));


Indent is off here. Currently seven spaces, should be four.,

jaredpar · 2018-05-31T21:17:57Z

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

+                 }
+                 else if (inaccessibleViaQualifier)
+                 {
+                       diagInfo = new CSDiagnosticInfo(ErrorCode.ERR_BadProtectedAccess, unwrappedSymbol, accessThroughType, this.ContainingType);


Indent is off here. currently six spaces, should be four.

jaredpar

Couple minor style issues but otherwise looks good.

agocke · 2018-06-01T01:17:14Z

src/Compilers/CSharp/Test/Emit/PrivateProtected.cs

                .VerifyDiagnostics(
-                // (12,11): error CS0122: 'Base.Field1' is inaccessible due to its protection level
+


This looks wrong. There are no IVT attributes or references in this compilation.

b.Field1 is trying to access a private protected field it has no access to. What error should be thrown instead?

The old message looks correct here. The current one doesn't make any sense. How could a compilation provide IVT access to itself?

Ok will change it back. The test passes only with the new error. How should we differentiate this from other cases?

agocke · 2018-06-01T01:19:09Z

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

+                            return false;
+                        }
+                    }
+                    return true;


This returns true even if there are no IVT attributes. The check should be that there is at least one attribute, but all of the public keys are wrong.

line 1221 is nested within the greater Accessibility check statement, and is only called if all of the SequenceEquals have returned false / the public keys are wrong. There is a return false at line 1223, outside of the accessibility check. Are you referring to different attributes?

DeclaredAccessibility just means what it was declared as, e.g. internal. No check on if it has IVT access.

Ah, then how does one test for attributes?

I think you can just check if the thing you're going to foreach over is empty before trying and return false if so.

agocke · 2018-06-01T19:43:59Z

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

@@ -1204,14 +1204,16 @@ internal SingleLookupResult CheckViability(Symbol symbol, int arity, LookupOptio

            bool IsBadIvtSpecification()
            {
+                IEnumerable<ImmutableArray<byte>> keys = unwrappedSymbol.ContainingAssembly.GetInternalsVisibleToPublicKeys(this.Compilation.AssemblyName);


You have to do this inside the if or, as the comment says, we'll stack overflow when binding attributes.

agocke

LGTM once unnecessary changes are reverted and tests pass.

agocke · 2018-06-01T21:45:59Z

src/Compilers/CSharp/Test/Emit/PrivateProtected.cs

@@ -266,7 +268,7 @@ void M()
    }
 }
 ";
-            CreateCompilation(source, parseOptions: TestOptions.Regular7_2)
+            CreateCompilation(source, parseOptions: TestOptions.Regular7_2, assemblyName: "Paul")


I think you can revert all changes to this file now.

agocke · 2018-06-01T21:52:00Z

src/Compilers/CSharp/Test/Emit/PrivateProtected.cs

@@ -352,7 +354,7 @@ private protected class Inner // error: protected not allowed in struct
    }
 }
 ";
-            CreateCompilation(source, parseOptions: TestOptions.Regular7_2)
+            CreateCompilation(source, parseOptions: TestOptions.Regular7_2, assemblyName: "Paul")


agocke · 2018-06-01T21:52:13Z

src/Compilers/CSharp/Test/Semantic/Semantics/AccessCheckTests.cs

@@ -633,7 +633,7 @@ public class C
 internal class D
 {
    static public int d_pub;
-}");
+}", assemblyName: "Paul");


agocke · 2018-06-01T21:53:17Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/BaseClassTests.cs

@@ -1642,7 +1643,7 @@ public void ProtectedAndInternalNestedBaseClass()
            var il = @"
 .assembly extern mscorlib { .ver 4:0:0:0 .publickeytoken = (B7 7A 5C 56 19 34 E0 89) } 

-.assembly '<<GeneratedFileName>>'
+.assembly 'Paul'


agocke · 2018-06-01T21:53:56Z

src/Scripting/VisualBasicTest/CommandLineRunnerTests.vb

@@ -145,7 +145,7 @@ System.Globalization.CultureInfo.DefaultThreadCurrentUICulture = System.Globaliz
 > System.Globalization.CultureInfo.DefaultThreadCurrentUICulture = System.Globalization.CultureInfo.GetCultureInfo(""de-DE"")
 > ? System.Math.PI
 3,1415926535897931
->", runner.Console.Out.ToString())
+> ", runner.Console.Out.ToString())


fayrose requested a review from a team as a code owner May 22, 2018 19:51

fayrose requested a review from agocke May 22, 2018 19:51

jcouv added the Area-Compilers label May 22, 2018

Fixed error 0281 and added test coverage.

37ed85e

fayrose force-pushed the larose-cs0281 branch from e11e6fc to 37ed85e Compare May 22, 2018 20:06

jaredpar reviewed May 23, 2018

View reviewed changes

CyrusNajmabadi reviewed May 23, 2018

View reviewed changes

Indentation fix

6eb8ea7

cston reviewed May 23, 2018

View reviewed changes

PublicKeyToString no longer an extension method; fixed diagnose in bi…

de25429

…nder lookup

agocke reviewed May 23, 2018

View reviewed changes

Extracted friendRefNotEqualToThis into local function

ee189b0

333fred reviewed May 23, 2018

View reviewed changes

Fixed usings

ab9faaf

jaredpar reviewed May 23, 2018

View reviewed changes

Double blank line fix for Jared

af6f5ac

agocke reviewed May 23, 2018

View reviewed changes

Lauren Rosenberg added 2 commits May 23, 2018 14:02

Resctructured diagnose block to increase readability

0b3c9f1

Fixed build errors

080de89

cston reviewed May 24, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs Outdated

{

Debugger.Launch();

return false;

}

Copy link

Member

cston May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this catch block necessary?

cston reviewed May 24, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs Outdated

{

try

{

if (inaccessibleViaQualifier)

Copy link

Member

cston May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The caller already checks this.

cston reviewed May 24, 2018

View reviewed changes

Requested changes

3191a12

agocke reviewed May 24, 2018

View reviewed changes

Stack overflow fix

e24f1a6

agocke reviewed May 29, 2018

View reviewed changes

Lauren Rosenberg added 3 commits May 29, 2018 15:51

Fix failing tests to conform to new error

cf7eb19

All local tests passing

200acb1

Extended new error message to PrivateProtected and InternalProtected

816472a

agocke reviewed May 30, 2018

View reviewed changes

Lauren Rosenberg added 3 commits May 30, 2018 11:46

PrivateProtected / InternalProtected Test changes

6f57496

Enhanced readability

56bb172

Bizarre rabbithole bug

7eba065

fayrose requested a review from a team as a code owner May 30, 2018 21:44

jaredpar reviewed May 31, 2018

View reviewed changes

jaredpar approved these changes May 31, 2018

View reviewed changes

Indent fixes

72b880a

agocke requested changes Jun 1, 2018

View reviewed changes

Re-added attribute check

28f0a80

agocke reviewed Jun 1, 2018

View reviewed changes

Lauren Rosenberg added 2 commits June 1, 2018 12:48

Restructured to prevent overflow

15176b2

Test fixes

6a36501

fayrose requested a review from a team as a code owner June 1, 2018 21:26

agocke approved these changes Jun 1, 2018

View reviewed changes

Lauren Rosenberg added 2 commits June 1, 2018 15:22

Removed Unnecessary changes

e9baab9

Oops

b095767

fayrose merged commit 1e8ba78 into dotnet:master Jun 2, 2018

fayrose deleted the larose-cs0281 branch June 2, 2018 07:05

agocke mentioned this pull request Aug 7, 2018

CS0281 error message has less details than native compiler #17322

Closed

		.VerifyDiagnostics(
		// (12,11): error CS0122: 'Base.Field1' is inaccessible due to its protection level

Diagnostic Improvements for error #0281 #27052

Diagnostic Improvements for error #0281 #27052

Conversation

fayrose commented May 22, 2018

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

agocke commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston May 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston May 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar May 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke commented May 30, 2018

jaredpar commented May 31, 2018

mmitche commented May 31, 2018

jaredpar May 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston May 24, 2018 •

edited

Loading

cston May 24, 2018 •

edited

Loading

jaredpar May 23, 2018 •

edited

Loading

jaredpar May 31, 2018 •

edited

Loading

agocke left a comment •

edited

Loading