Fix - local function - refactorings not available when selection is used

[petrroll]

WIP on fixing low-hanging fruits from #35180

Includes some new Helpers for determining whether current position/span is desirable for refactoring.

[petrroll]

Rework the fix with FindNode. Please check.

[CyrusNajmabadi]

In regards to the helper, don't we also want to handle cases like these:

      vo[|id Whatever()
        {
        }
|]

I don't think you have to support that to begin with.

[CyrusNajmabadi]

Ok, taking that back. FullSpan contains some before/after whitespace but doesn't really like newspaces.

Yes, this is the design of 'FullSpan'. The way tokens work in Roslyn is that the compiler does this:

1. it consumes all trivia until it reaches a token. This is the leading trivia.
2. it eats the text of the token itself.
3. it consumes trivia until it reaches the first newline. This is the trailing trivia
4. Go to '1'

This is a totally consistent view of hte world at the syntactic model level. Features can trust that this is exactly how tokens/nodes will be represented all the time.

Now, given that consistency, features can write helpers accordingly that then map between what we want for the user and that syntactic representation that the parsers produce.

[petrroll]

@CyrusNajmabadi is it always true that parent's Span contains all children Spans? I.e can I be sure that when traversing the tree upwards the Spans will monotonously grow/stay the same?

[CyrusNajmabadi]

@CyrusNajmabadi is it always true that parent's Span contains all children Spans? I.e can I be sure that when traversing the tree upwards the Spans will monotonously grow/stay the same?

Yes.

[petrroll]

Yep, figure that could be the issue as well (since I don't even have the file locally). Working on it ATM.

[petrroll]

Ok, after rebasing (and some fun with git & VS not liking that many changes while being open) I've found the code in question and it has empty Initialize which I suppose is the issue. Should I simply add pragmas ignore to ignore the two warnings?

Or should I go with what the codefixes suggest? (I haven't looked into how the diagnosticssuppressor works so have no idea what option is the correct one).

[petrroll]

Following code is suggested by codefix:

And what about concurrent exec?

        public sealed override void Initialize(AnalysisContext context)
        {
            context.EnableConcurrentExecution();
            context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics);
        }

I'm pretty sure we want to neither REportDIagnostics (given what the comment above says // Disallow suppressors from reporting diagnostics or registering analysis actions nor analyze. So I suppose the Configure be with GeneratedCodeAnalysisFlags.None?

[mavasani]

@petrroll Thanks for the investigation. I think we should handle this as part of merge PR that merges master into release/dev16.3-preview1.

@JoeRobich - are we seeing this build issue in the merge PR as well? If not, @petrroll it might be best to create a separate PR for this fix as it will make reviewing easier. Note that this is a change in compiler layer, so you would need 2 signoffs from the roslyn-compiler team.

[mavasani]

Or should I go with what the codefixes suggest? (I haven't looked into how the diagnosticssuppressor works so have no idea what option is the correct one).

@petrroll I just added that feature into release\dev16.3-preview1 branch last week, and @sharwell separately moved our analyzer packages forward to 2.9.3 in master branch last week. The latter brought in the new analyzer that flags RS1025 and RS1026. I believe the correct fix is to just suppress this diagnostic for the DiagnosticSuppressor.Initialize override.

[petrroll]

As for the RS1015 it seems to me that it should be called with context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);. No need to suppress it (I mean, the comment in the type says no diagnostics / analysis should be registered anyway). As for concurrent execution, yep, that seems as if that should be suppressed.

[mavasani]

As for the RS1015 it seems to me that it should be called with context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);. No need to suppress it. As for concurrent execution, yep, that seems as if that should be suppressed.

Sounds reasonable to me.

[petrroll]

Should I create a PR with fix for that or will you handle that?

[mavasani]

Should I create a PR with fix for that or will you handle that?

I was hoping this would be handled as part of regular merge PRs from master to release/dev16.3-preview1. @JoeRobich?

[mavasani]

@petrroll #36726