Note: my only concern with this being at the compiler level is: does this accurately represent accessibility across both languages. In the IDE it was fine to have a common 'good enough' impl that behaved reasonably well for C#/VB. In other words, i never vetted it to make sure it was doing the right thing for VB, with the assumption that if it wasn't, it would likely be so corner case as to not matter for IDE features.

As a compiler API, i would like things to have a higher bar of accurately reflecting the language. If this does that for VB and C#, then i'm happy :) #Resolved

my only concern with this being at the compiler level is: does this accurately represent accessibility across both languages.

It accurately reflects C# and CLR accessibility. It is simpler than the compiler internal implementations because it doesn't have to protect from cycles that can occur when it is used within the compiler, and it doesn't have to distinguish between different reasons for access failure.

If VB differs, then it can't do both C# and VB at the same time. Having said that, I don't believe VB differs. #Resolved

If VB differs, then it can't do both C# and VB at the same time.

Yeah... that would be the downside (and would block usage from the IDE, thus defeating hte purpose).

Basically, i'm ok with this if VB is ok with this. #Resolved

and CLR accessibility

That's a strong sign this would be ok. Since the CLR is the common substrate between the languages, if this logic is at least providing those semantics, then i think this is fine no matter what. #Resolved

FYI, it looks like the VB implementation of accessibility mirrors the C# implementation, even when that conflicts with the language specification:

            ' Protected is quite confusing. It's a two-fold test:
            '    1) The class access is from ("withinType") is, or is inside, 
            '        a class derived from "originalContainingType"
            '    2) If there is a qualifier, and the member is not shared, 
            '       the qualifier must be an instance of the derived
            '       class in which the access occurred (any construction thereof)
            '
            ' The VB language spec describes a third test:
            '    3) If there is a qualifier, and the member is not shared, the qualifier
            '       must be of the exact construction of the derived class from which access occurred.
            ' But Dev10 actually implements something rather different:
            '    3) If there is a qualifier, for any member, shared or not, the construction of "originalContainingType" that
            '       the qualifier type inherits from is the same as the construction of "originalContainingType" that the 
            '       class through which access occurred.
            ' This rule (either the spec'd or Dev10 version) is intentionally not implemented here. See bug 4107.

#Resolved

@AlekseyTs I understand that you'd like more "verification" about compilations, but this API isn't intended to be bound to particular compilations (there are multiple symbol arguments and they can be from different compilations); compilations are not part of its contract. You say that the implementation as written "is likely to produce false negative results". Can you please identify specific scenarios in which it produces an incorrect result? #Resolved

test windows_release_vs-integration_prtest please #Resolved

Note: the IDE creates symbols that do not belong to any compilation. We do this as part of many features. For example, we synthesize up methods during 'generate method' that are a real IMethodSymbol that are not backed by a C# or VB MethodSymbol. These symbols then flow around many parts of our stack, including parts that want to potentially do accessibility checks.

Having a language agnostic API allows us to pass this data around without having to ensure that all parts of the stack have to be agreeing on the same language. And it allows accessibility checks to work even with symbols that we are literally synthesizing out of fresh air.

--

But even in the case where we are not creating totally new symbols, there are still cases where this functionality is valuable. here's a good end-to-end example with just C#/VB symbols.

Say you have two projects, a C# and a VB project. And you want to add 'add using with project reference'. The search finds a potentially applicable symbol in teh VB project, but then wants to know if this symbol would be accessible at the location you're at in the C# project. Note: we cannot 'map' the VB symbol into the C# project because there is literally no project reference between the two (that's what this feature is trying to decide if it should do). So it wants to just say "would this symbol be accessible to the target language?" in an efficient manner. i.e. we do not want to build up compilations, map symbols, and then ask queries in that fork when we can just ask a cheap question about two symbols.

#Resolved

The intent of this API is to answer accessibility questions on ISymbols, which contain enough information to answer accessibility questions. There is no need for us to delegate to language-specific symbol classes. #Resolved

The intent of this API is to answer accessibility questions on ISymbols, which contain enough information to answer accessibility questions. There is no need for us to delegate to language-specific symbol classes.

I think there is no need to write new code, a third implementation of accessibility check, which actually doesn't provide benefits it might appear to provide. It looks like the implementation relies on symbol comparison, symbols from different languages are never equal. So, what is the point in having shared implementation that simply isn't going to have consistent behavior across all the scenarios it is expected to handle? If we don't want to put the method on a Compilation, we can simply add it as a real member on ISymbol interface, we add new APIs to symbol interfaces all the time. Why this one is special? Then, as with other similar APIs, we check that input belongs to the language and call implementation we already have. This, however, won't allow us to check whether the symbols belong to the same "universe". Note, that symbols that belong to the same language and represent the same entities might come from different "universes" and, therefore, will not be considered equal. A type symbol for System.Console obtained from one compilation might not be equal to symbol for System.Console obtained from another compilation. The shape of the API is likely to lead people to believe that it is not important where symbols are coming from, but it actually is extremely important. Some very simple scenarios, with public symbols, are likely to produce expected results, but the failures will actually come for more interesting scenarios. That said, I believe it is better to structure the API in the way that sets right expectations and leads consumers down the path of success rather than failure. Therefore, it feels bad to make it appear as though the API works reliably across languages, when it doesn't. That should be clearly documented and consumers should be prevented to call this API while mixing symbols from different languages. I would also prefer us to go farther and actually do more verification, making sure that symbols actually belong to the same compilation, only then we can guarantee correctness of the result across all scenarios. #Resolved

@CyrusNajmabadi

Having a language agnostic API allows us to pass this data around without having to ensure that all parts of the stack have to be agreeing on the same language. And it allows accessibility checks to work even with symbols that we are literally synthesizing out of fresh air.

I do not believe the proposed implementation is going to give you all that. Also, keep in mind that this is a public API on symbols, the fault tolerance is much lower for it and correctness and usability requirements are much higher. As long as the code is internal implementation in IDE layer, the amount of consumers "suffering" from its incorrect/unreliable behavior is relatively small, but we are about to change that, making "promises" that are not fulfilled. #Resolved

I do not believe the proposed implementation is going to give you all that.

Note, as part of the PR Neal did remove the near-equivalent version in teh IDE layer and replaced those existing calls with all the calls to his new API. So it does appear to give us this sufficient level of accessibility testing.

Therefore, it feels bad to make it appear as though the API works reliably across languages, when it doesn't.

Note: the primary use case of this API was intended to support answering these questions for cross project scenarios. That's how the IDE has been using the existing extension for many years now at this point.

Also, keep in mind that this is a public API on symbols, the fault tolerance is much lower for it and correctness and usability requirements are much higher.

I agree with this.

making "promises" that are not fulfilled.

This part i'm less sure about, but i will leave neal to answer. Specifically, the satisfactory part for me was that "It accurately reflects ... CLR accessibility." If these are the rules for CLR accessibility, it feels like they can hopefully be exposed without actually needing or depending on specific symbol implementations. If specific symbol implementations are necessary in your opinion, it would be good to understand where.

From your response on the PR one good example of a strange place is return toAssembly.GivesAccessTo(fromAssembly); It would be interesting to actually enumerate those cases and see if they can actually be written in a way that is truly symbol agnostic. If so, then we can accomplish both sets of overall goals as far as i can tell.

#Resolved

It looks like the implementation relies on symbol comparison, symbols from different languages are never equal.

That looks like a critique that doesn't torpedo the ship, but merely needs to be something addressed as part of the PR. One question is: is it fundamental to accessibility checking that one be able to do symbol comparison? Or can it be done without. If the former, this appraoch may not be viable at all. If the latter, that just indicates work to be done here to improve things.

Note: a concern of mine would be protected scenarios. That's definitely a case that seems like it could be problematic.

Some very simple scenarios, with public symbols, are likely to produce expected results, but the failures will actually come for more interesting scenarios.

IT would be interesting to enumerate those scenarios. That would help brainstorm to see if they really are deal-breakers, or if there are simple and effective solutions that could be created to validate that they are working properly. #Resolved

It looks like this PR lacks adequate testing and this is a show stopper on itself #Resolved

@AlekseyTs Not sure what you mean about testing - there is test coverage of every branch in the implementation. However, I will add a layer of testing based on mock symbols to ensure we do not depend on language-specific implementations. #Resolved

there is test coverage of every branch in the implementation

This on its own is not an indication of presence of adequate testing.

Here some of the things that I think should be present and is a good start:

• All scenarios should be covered by targeted tests under Compilers.
• All branches should be covered for all language specific symbols. Mock symbols might be useful, but this is lower in priority
• Cross language scenarios should be covered in all possible combinations. There are three inputs, the test matrix should be covered.
• Cross compilation scenarios should be covered when symbols are not shared between them, the same test matrix should be covered. #Resolved

Cross language scenarios should be covered in all possible combinations. There are three inputs, the test matrix should be covered.

I think this is a very good idea. At the very least it will uncover bugs/issues elsewhere that we can drive solutions for. For example, it would have found that our IAssembly symbols are not behaving as currently doc'ed.

Even if these bugs have existed before, it would still be good to use this time as an opportunity to identify them and squash them so that we can have a veyr high quality implementation that people can know they can depend on.

Note: we can discuss the necessity of fixing any bugs we find in other areas of the codebase if/when we find anything. It could be acceptable to just file bugs on those parts and accept that those bugs have leaked into higher level APIs with the expectation that they could be fixed at some later point in time. But it would still be good to know about them and have a good test suite that will appropriately hit those codepaths. #Resolved

One thing I should mention is that interactive and scripting implementations relax (i.e. ignore some of) the CLR rules for access checking. This implements the rules corresponding to what the compilers do.

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

Adding the following doc comments:

        /// <remarks>
        /// The following areas of imprecision may exist in the results of this API:
        /// <para>For assembly identity, we depend on Equals <see cref="IEquatable{T}.Equals(T)"/>/>.
        /// Assembly symbols that represent the same assembly imported into different language compilers
        /// may not compare equal, and this may prevent them from appearing to be related through
        /// this API. See https://github.com/dotnet/roslyn/issues/26542 .</para>
        /// <para>We compare <see cref="INamedTypeSymbol"/> based on the identity of the containing
        /// assembly (see above) and their metadata name, which includes the metadata name of the enclosing
        /// namespaces. Due to the behavior of the VB compiler (https://github.com/dotnet/roslyn/issues/26546)
        /// it merges namespaces when importing an assembly. Consequently, the metadata name of the namespace
        /// may not be correct for some of the contained types, and types that are distinct
        /// may appear to have the same fully-qualified name. In that case this API may treat them as the same type.</para>
        /// </remarks>

#Closed

An alternative would be to compare (original) type symbols using object.Equals() rather than comparing metadata names. That would change issue (2) in the doc comments. #WontFix

I will be changing the test for assembly identity to use IAssembly.Identity.Equals, since that is what is specified and used by the compilers and the runtime to determine when assemblies are the same. #Closed

API review yesterday moved IsAccessibleWithing to Compilation. That is a large change from this API, so I'll close this and reopen a new PR.