Added “SemanticModel.WithSuppressAccessChecks” extension method. #26

OmerRaviv · 2015-01-18T08:53:05Z

The WithSupressAccessChecks method creates a SemanticModel that ignores accessibility rules when answering semantic questions.

This allows consumers to ask semantic questions using the same semantic rules as the ones used in debugger’s Expression Evaluator, where accessibility rules do not matter. This API is an absolute necessity for 3rd parties (such as OzCode) who want to create debugger-related productivity tools on top of Roslyn.

Added unit tests for C# and VB.NET to cover both regular and speculative analysis.

fixes #378

gafter · 2015-02-04T02:16:38Z

src/Compilers/Core/Portable/Compilation/SemanticModel.cs

@@ -63,6 +63,16 @@ public SyntaxTree SyntaxTree
        protected abstract SyntaxTree SyntaxTreeCore { get; }

        /// <summary>
+        /// Returns true if this is a SemanticModel that ignores accessibility rules when answering semantic questions.
+        /// </summary>
+        public bool IsSuppressingAccessChecks { get; protected set; }


The setter should be internal.

AnthonyDGreen · 2015-02-06T00:55:07Z

src/Compilers/Core/Portable/Compilation/SemanticModel.cs

+        /// <summary>
+        /// Creates a SemanticModel that ignores accessibility rules when answering semantic questions.
+        /// </summary>
+        public abstract SemanticModel WithSuppressAccessChecks();


I'd much rather expose this as a new optional parameter added to the existing Compilation.GetSemanticModel overloads which takes a Boolean flag for whether accessibility checks should be suppressed for the purpose of lookup which defaults to false. I suggest bool suppressAccessChecks = false.

OmerRaviv · 2015-02-09T14:36:26Z

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

gafter · 2015-02-10T21:07:13Z

Note that this fixes #378

AnthonyDGreen · 2015-02-10T23:34:19Z

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

mattwar · 2015-02-10T23:47:45Z

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.

gafter · 2015-02-12T00:21:55Z

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

OmerRaviv · 2015-02-12T04:58:27Z

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

gafter · 2015-02-17T04:53:06Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

@@ -2747,6 +2748,13 @@ internal Conversion ClassifyConversionForCast(int position, ExpressionSyntax exp
        /// <param name="typeParameter"></param>
        public abstract ITypeParameterSymbol GetDeclaredSymbol(TypeParameterSyntax typeParameter, CancellationToken cancellationToken = default(CancellationToken));

+        protected BinderFlags GetSemanticModelBinderFlags()


This should be internal, not protected, as it should not be part of the publicly visible API.

gafter · 2015-02-17T04:57:49Z

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

Calling GetSemanticModel with suppressAccessChecks=true creates a SemanticModel that ignores accessibility rules when answering semantic questions. This allows consumers to ask semantic questions using the same semantic rules as the ones used in debugger’s Expression Evaluator, where accessibility rules do not matter. Added unit tests for C# and VB.NET to cover both regular and speculative analysis.

tmat · 2015-02-18T17:10:17Z

Looks reasonable.

@rchande @amcasey Could we use this in our EE completion provider? The EE allows evaluating internal members but completion doesn't show them, which is a bummer since the IDE actively fights you when you try to write them.

jaredpar · 2015-02-18T17:11:33Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

@@ -51,6 +51,7 @@ internal abstract class CSharpSemanticModel : SemanticModel
        /// </summary>
        internal abstract CSharpSyntaxNode Root { get; }

+
        // Is this node one that could be successfully interrogated by GetSymbolInfo/GetTypeInfo/GetMemberGroup/GetConstantValue?


Nit: double blank line.

rchande · 2015-02-18T17:15:26Z

@tmat Since it's just a public API on Compilation, we could have completion use this during debug time. Does this automatically work with Speculative SemanticModels?

jaredpar · 2015-02-18T17:17:21Z

src/Compilers/Core/Portable/Compilation/SemanticModel.cs

@@ -63,6 +63,14 @@ public SyntaxTree SyntaxTree
        protected abstract SyntaxTree SyntaxTreeCore { get; }

        /// <summary>
+        /// Returns true if this is a SemanticModel that ignores accessibility rules when answering semantic questions.
+        /// </summary>
+        public virtual bool HasAccessChecksSuppressed


Can we name this SuppressAccessChecks?

The pattern of SuppressY is used a number of times in our code base both internally and publically. The pattern of HasYSuppressed does not appear anywhere I could find though we do have a few occurrences of IsYSuppressed but they are all methods and internal.

That pattern seems to be more consistent with other public, and even internal, APIs that we expose.

@gafter what do you think?

What about IgnoresAccessibility?

jaredpar · 2015-02-18T17:18:26Z

👍

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!

gafter · 2015-02-18T17:35:14Z

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

@AnthonyDGreen what do you think?

jaredpar · 2015-02-18T17:39:23Z

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.

OmerRaviv · 2015-02-18T17:56:43Z

@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.

AnthonyDGreen · 2015-02-18T19:04:30Z

@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.

jaredpar · 2015-02-18T19:10:22Z

@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.

gafter · 2015-02-18T20:20:11Z

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.

jaredpar · 2015-02-18T20:27:33Z

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

gafter · 2015-02-18T22:26:36Z

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

jaredpar · 2015-02-18T23:53:11Z

Internal or not I think we should be consistent.

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

From: Neal Gafter notifications@github.com
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.

AnthonyDGreen · 2015-02-19T00:31:42Z

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.

gafter · 2015-02-19T17:21:44Z

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.

jaredpar · 2015-02-19T17:22:19Z

Thanks fine by me.

Added “SemanticModel.WithSuppressAccessChecks” extension method.

terrajobst · 2015-03-10T18:57:12Z

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) { ... }

theoy added Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. labels Jan 18, 2015

theoy added this to the 1.1 milestone Jan 18, 2015

theoy added the Enhancement label Jan 19, 2015

theoy assigned gafter Jan 20, 2015

gafter reviewed Feb 4, 2015
View reviewed changes

AnthonyDGreen reviewed Feb 6, 2015
View reviewed changes

gafter modified the milestones: 1.0-rc2, 1.1 Feb 8, 2015

gafter added 1 - Planning 4 - In Review A fix for the issue is submitted for review. and removed 1 - Planning labels Feb 9, 2015

gafter added the cla-signed label Feb 12, 2015

gafter reviewed Feb 17, 2015
View reviewed changes

OmerRaviv force-pushed the supress-accessibility-checks branch from 256fc68 to f91f779 Compare February 18, 2015 11:38

jaredpar reviewed Feb 18, 2015
View reviewed changes

gafter added a commit that referenced this pull request Feb 19, 2015

Merge pull request #26 from OmerRaviv/supress-accessibility-checks

29a3b46

Added “SemanticModel.WithSuppressAccessChecks” extension method.

gafter merged commit 29a3b46 into dotnet:master Feb 19, 2015

gafter removed the 4 - In Review A fix for the issue is submitted for review. label Feb 19, 2015

tmat added a commit to tmat/roslyn that referenced this pull request Nov 2, 2015

Use nuget blob dotnet#26

83a7c54

jaredpar unassigned gafter Nov 23, 2015

jaredpar removed this from the 1.0-rc2 milestone Nov 23, 2015

jaredpar added Feature Request and removed Enhancement labels Nov 23, 2015

davkean added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Dec 16, 2015

AdamSpeight2008 added a commit to AdamSpeight2008/roslyn-AdamSpeight2008 that referenced this pull request Feb 20, 2017

Use Multi-Line String LIteral in VB Unit Tests (dotnet#26)

0c4613f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added “SemanticModel.WithSuppressAccessChecks” extension method. #26

Added “SemanticModel.WithSuppressAccessChecks” extension method. #26

OmerRaviv commented Jan 18, 2015

gafter Feb 4, 2015

AnthonyDGreen Feb 6, 2015

OmerRaviv commented Feb 9, 2015

gafter commented Feb 10, 2015

AnthonyDGreen commented Feb 10, 2015

mattwar commented Feb 10, 2015

gafter commented Feb 12, 2015

OmerRaviv commented Feb 12, 2015

gafter Feb 17, 2015

gafter commented Feb 17, 2015

tmat commented Feb 18, 2015

jaredpar Feb 18, 2015

rchande commented Feb 18, 2015

jaredpar Feb 18, 2015

gafter Feb 18, 2015

jaredpar commented Feb 18, 2015

gafter commented Feb 18, 2015

jaredpar commented Feb 18, 2015

OmerRaviv commented Feb 18, 2015

AnthonyDGreen commented Feb 18, 2015

jaredpar commented Feb 18, 2015

gafter commented Feb 18, 2015

jaredpar commented Feb 18, 2015

gafter commented Feb 18, 2015

jaredpar commented Feb 18, 2015

AnthonyDGreen commented Feb 19, 2015

gafter commented Feb 19, 2015

jaredpar commented Feb 19, 2015

terrajobst commented Mar 10, 2015

Added “SemanticModel.WithSuppressAccessChecks” extension method. #26

Added “SemanticModel.WithSuppressAccessChecks” extension method. #26

Conversation

OmerRaviv commented Jan 18, 2015

gafter Feb 4, 2015

Choose a reason for hiding this comment

AnthonyDGreen Feb 6, 2015

Choose a reason for hiding this comment

OmerRaviv commented Feb 9, 2015

gafter commented Feb 10, 2015

AnthonyDGreen commented Feb 10, 2015

mattwar commented Feb 10, 2015

gafter commented Feb 12, 2015

OmerRaviv commented Feb 12, 2015

gafter Feb 17, 2015

Choose a reason for hiding this comment

gafter commented Feb 17, 2015

tmat commented Feb 18, 2015

jaredpar Feb 18, 2015

Choose a reason for hiding this comment

rchande commented Feb 18, 2015

jaredpar Feb 18, 2015

Choose a reason for hiding this comment

gafter Feb 18, 2015

Choose a reason for hiding this comment

jaredpar commented Feb 18, 2015

gafter commented Feb 18, 2015

jaredpar commented Feb 18, 2015

OmerRaviv commented Feb 18, 2015

AnthonyDGreen commented Feb 18, 2015

jaredpar commented Feb 18, 2015

gafter commented Feb 18, 2015

jaredpar commented Feb 18, 2015

gafter commented Feb 18, 2015

jaredpar commented Feb 18, 2015

AnthonyDGreen commented Feb 19, 2015

gafter commented Feb 19, 2015

jaredpar commented Feb 19, 2015

terrajobst commented Mar 10, 2015