Thanks a lot for the feedback, @AnthonyDGreen. That's indeed a much cleaner solution. Fixed.

Note that this fixes #378

@tmat, @KevinH-MS, @cston, @ManishJayaswal, does this aid or address any debugging/REPL scenarios that also require bypassing access checks in the SemanticModel?

A debugging scenario is freer with accessibility than the REPL scenarios we've discussed. One REPL scenario discussed was to have an option that allowed the REPL or script code to access internals as if internals-visible-to had been set. Note, a REPL might eventually be used in a debugging scenario, so this could come in to play for that. Note, these REPL scenarios require more than just a binder modification, they need a runtime change.

@OmerRaviv Can you please "sign" a contributor's agreement starting at https://cla.dotnetfoundation.org/

@gafter I've signed the dotnetfoundation CLA on the 18th of January using the email address that's associated with my GitHub account, [email protected], and got the approval the next day.

This pull request failed its automatic merge and test. Please re-merge with the latest and resubmit the pull request.

👍

I had one bit of feedback about the choice of API name here. I think SuppressAccessChecks is more consistent with existing patterns HasAccessChecksSuppressed. Wanted to get others feedback on that before merging.

Other than that though it looks great!

SuppressAccessChecks sounds a bit more like a command than a test.

@AnthonyDGreen what do you think?

Merging a couple of comment threads here. The two alternate proposals for name are:

• SuppressAccessChecks
• IgnoresAccessibility

I am fine with either. Both have precedent within the Roslyn code base and fit into existing patterns which was my primary concern.

Small nit is I would use IgnoreAccessibility over IgnoresAccessibility. There are no cases I could find of a plural form of ignores but plenty where the singular variety is used.

@rchande Yes, this automatically supports Speculative SemanticModels, see unit tests for proof :)
@rchande @tmat @amcasey Yes, I think it would make sense to combine this functionality with #423, so that you can get IntelliSense to include inaccessible members while debugging.

@jaredpar @gafter @AnthonyDGreen I don't really have an opinion regarding naming either way, please let me know once you've reached consensus and I'll change it.

@jaredpar, I don't think we have that many Boolean flags in the API but in the past I've used "the if test". How does checking the flag look in an if:

if (semanticModel.HasAccessChecksSuppressed) { .. }
if (semanticModel.IgnoresAccessibility) { ... }
if (semanticModel.SuppressAccessChecks) { ... }

I think either of the first two read better in that context. This is also what leads me to prefer 'ignores' over 'ignore'. We had a similar discussiong when we named ControlFlowAnalysis/DataFlowAnalysis.Succeeded vs Success. If we're applying this inconsistently we should review the various cases holistically.

@terrajobst for weigh in on naming.

@AnthonyDGreen the choice of Ignores vs. Ignore doesn't really matter to me. All I care about is consistency in the choices. Today we do have APIs in the singular so if we make this plural I prefer we make all of the other plural as well (even if the APIs are internal). The same is true for HasYSuppressed, it breaks an existing established pattern.

I can't find examples where we use the "singular" except in the imperative sense (of instructing the API to ignore something), such as LookupOptions.IgnoreAccessibility. This property test the setting.

Here are the cases I could find.

Singular:

• AbstractRegionDataFlowPass.IgnoreOutSemantics
• DataFlowPass.IgnoreOutSemantics
• UnassignedVariablesWarker.IgnoreOutSemantics
• IgnoreBaseClassesInLookup
• CandidateAnalysisResult.IgnoreExtensionMethods
• ComparisonOptions.IgnoreCase
• ComparisonOptions.IgnoreAssemblyKey
• SuppressOptions.IgnoreElastic

Plural:

• Binder.IgnoresAccessibility

The only ones of those that are public are options (commands) to ignore something.

Internal or not I think we should be consistent.

JaredPar from a phone
http://blog.paranoidcoding.com/

From: Neal Gafter [email protected]
Sent: Wednesday, February 18, 2015 2:26:37 PM
To: dotnet/roslyn
Cc: Jared Parsons
Subject: Re: [roslyn] Added “SemanticModel.WithSuppressAccessChecks” extension method. (#26)

The only ones of those that are public are options (commands) to ignore something.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/26#issuecomment-74961945.

They're in different contexts. One is a flag on SemanticModel describing a property of the semantic model. The Ignores isn't plural, it's third person singular. I ignore, you ignore, he/she/it ignores. This is the same pattern we use for SemanticModel.IsSpeculativeSemanticModel method (or Object.Equals). I am, you are, he/she/it is.

This is a different grammatical context than the parameter which one passes in to GetSemanticModel to cause accessibility to be ignored - ignoreAccessibility. That's describing an action that should be taken (sort of a second person command) and options follow that same convention.

The problem is that depending on where the code is written a piece of data will need to be specified or described in different persons. To the person acquiring the SemanticModel it's 2nd - ignoreAccessibility, within the SemanticModel it would be 1st - IgnoreAccessibility, and outside of the SemanticModel it will be in the 3rd - IgnoresAccessibility. Things get even muddier when properties are read-write because getting is usually in the 3rd and setting is usually in the 2nd. We can't even just standardize on one because - Object.Equals is 3rd person, and IsSpeculativeSemanticModel is 3rd person. I'm not even sure what the latter would be in the 1st or 2nd persons - BeSpeculative, Speculate, AmSpeculative, or just Speculative?

So which name is used where depends heavily on who the primary consumer is. The parameters and options are primarily commands for someone else (e.g. the Compilation, the parser) and should be written in the 2nd person, and the properties on SemanticModel are primarily attributes that should be written in the 3rd person.

I agree that the internal cases should be made consistent if they are in a context where that reasoning applies.

I am going to accept this pull request as-is and then submit a separate change to rename the public member. Please say something now if that sounds like a bad plan.

Thanks fine by me.

Sorry to be super later but hey -- better late than never :-)

@terrajobst for weigh in on naming.

I'm with Jared. Consistency is key. As far as imperative vs. descriptive style goes: we have both. It usually depends on context. In general, it's good to design booleans to make them read nice in if checks. I believe the ones discussed all satisfy this:

if (semanticModel.HasAccessChecksSuppressed) { .. }
if (semanticModel.IgnoresAccessibility) { ... }
if (semanticModel.SuppressAccessChecks) { ... }