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.

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

indentation seems off here...

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;

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.

Looks like the old whitespace was correct

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

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.

Accidental whitespace change.

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

Can ContainingAssembly.Name be null here?

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.

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

Nit: double blank line above. #Resolved

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

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

Is this catch block necessary?

The caller already checks this.

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

No need to check keys beyond the first match.

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

Should the class be named PublicClass?

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();
        ...
    }
}";

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

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.

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;

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

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)

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

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

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

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

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?

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.

Pushed

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

pushed