Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

'using' CodeActions not available if #line hidden is present anywhere in the file #40655

Closed
ryanbrandenburg opened this issue Dec 30, 2019 · 16 comments
Labels
Area-IDE Resolution-By Design The behavior reported in the issue matches the current design

Comments

@ryanbrandenburg
Copy link
Contributor

Repro steps:

  1. dotnet new console
  2. In Program.cs remove using System;
  3. Hover over the now-broken Console.WriteLine and see that using System; is a CodeAction option.
  4. Add #line hidden just below Console.WriteLine.
  5. Repeat 3 and see that the using System; option is now gone.

This affects us because when Razor generates code for the C# portions of it #line default #line hidden and #line X are used.

CC @NTaylorMullen

@ryanbrandenburg ryanbrandenburg changed the title 'using' CodeActions not available if #line hidden is present in the file 'using' CodeActions not available if #line hidden is present anywhere in the file Dec 30, 2019
@CyrusNajmabadi
Copy link
Member

This is by design. the #line pattern indicates to us that the area we would add hte using to is hidden from the user (and thus out of their control and under the contorl of another tool). I believe for this to work, you would need to add a #line default on the first line to inform the IDE that the usings section is visible to the user. otherwise, based on how old tools worked, we will assume it was hidden.

@tmat
Copy link
Member

tmat commented Jan 2, 2020

@CyrusNajmabadi I don't think this is by design. In the example above #line hidden is below the line that should offer the code action.

image

@CyrusNajmabadi
Copy link
Member

No... It's by design. This was the pattern other generators used. And we had to preserve compat with them

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jan 2, 2020

From our code, we say:

            return text.OverlapsHiddenPosition(span, (position, cancellationToken2) =>
                {
                    // implements the ASP.NET IsHidden rule
                    var lineVisibility = tree.GetLineVisibility(position, cancellationToken2);
                    return lineVisibility == LineVisibility.Hidden || lineVisibility == LineVisibility.BeforeFirstLineDirective;
                },
                cancellationToken);

...

        /// <summary>
        /// The line is located before any #line directive and there is at least one #line directive present in this syntax tree.
        /// This enum value is used for C# only to enable the consumer to define how to interpret the lines before the first
        /// line directive. 
        /// </summary>
        BeforeFirstLineDirective = 0,

So this is how ASP.net did things. The workaround is to place a #line directive on line 0. If we change our behavior here, we will break old asp.net projects. It's unfortunate, but this behavior is needed for legacy compat with too much code.

@tmat
Copy link
Member

tmat commented Jan 2, 2020

OK, then I'd not call this by design, but rather a hack for legacy ASP.NET. Can we only use the hack when we are in legacy ASP.NET project? Because this is not how the compiler defines non-user code. It seems unfortunate to perpetuate incorrect behavior just because legacy ASP.NET didn't generate correct #line directives.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jan 2, 2020

OK, then I'd not call this by design, but rather a hack for legacy ASP.NET

I mean... it was the design of #line. That's how they used it and how we always supported the concept. Since this entered the IDE (and throughout roslyn) it's always worked this way.

All the teams (and IDE) agreed on this behavior. Compiler even exposes this in the API because it's so entrenched as the pattern.

Can we only use the hack when we are in legacy ASP.NET project?

I'd much rather have everyone just use hte same pattern. :)

Having to support multiple patterns is just extra complexity for all our code. If something is the defacto pattern, i'd like to just have that.

--

Note: i strongly feel this way because IDE is always expected to deal with these teams changing behavior and hten supporting that behavior forever. We have waaay too many cases where we end up having to have codepaths to support N different project systems, M different 'code-projection' systems, etc. etc. The teams just move on, but IDE needs to absorb all the complexity forever. It's a large maintenance burden that actively makes future work more costly to maintain.

@tmat
Copy link
Member

tmat commented Jan 2, 2020

All the teams (and IDE) agreed on this behavior.

That's too bad, since the #line directive is definitely not defined this way :(.

@CyrusNajmabadi
Copy link
Member

That's too bad, since the #line directive is definitely not defined this way :(.

Welcome to the old days of the project when Compiler and IDE didn't talk to each other and IDE had to reimplemet hte entire compiler API :D

@NTaylorMullen
Copy link
Contributor

NTaylorMullen commented Jan 6, 2020

Sorry for the late reply, I've been sick.

I believe for this to work, you would need to add a #line default on the first line to inform the IDE that the usings section is visible to the user. otherwise, based on how old tools worked, we will assume it was hidden.

@CyrusNajmabadi I'm trying to understand where adding a #line default would make this work ever

image

And just for context, in Razor file if you do @{ Console.WriteLine("..."); } it generates as the following boiler plate (does not allow usings directives):

#nullable restore
#line 23 "C:\Users\nimullen\source\repos\Preview1_31BlazorServerSide\Preview1_31BlazorServerSide\Pages\_Host.cshtml"
    Console.WriteLine("...");

#line default
#line hidden
#nullable disable

@CyrusNajmabadi
Copy link
Member

I'd have to debug through to see waht was up here.

@tmat
Copy link
Member

tmat commented Jan 6, 2020

Somewhat unrelated but why switching to default and then immediately to hidden? #line default seems superfluous.

#line default
#line hidden

@NTaylorMullen
Copy link
Contributor

Somewhat unrelated but why switching to default and then immediately to hidden? #line default seems superfluous.

Most likely an artifact of old Razor where the current VS Razor formatting engines expect our C# to look a certain way. We've tried changing the generated Razor C# in the past and have ended up paying dearly in the formatting front for the dumbest of reasons 😄

@NTaylorMullen
Copy link
Contributor

@CyrusNajmabadi just to clarify, that's definitely not expected right?

@CyrusNajmabadi
Copy link
Member

So this works for me:

image

Looks like teh case that currently isn't supported is when there's zero usings in the file. however, that's never been the case for any asp generators using #line. There are always some usings in scope.

You could def create a PR to fix this specific issue if it's a problem for you. But i'm curious what the use case is where a templated language isn't even inserting using System; into the code for users.

@NTaylorMullen
Copy link
Contributor

Ooo interesting, definitely not a blocker when there's 0 usings. That's what I get for trying to get a "minimal" repro 😄

At design time we can start generating a line default around our usings sections because it's not like you can debug over those anyhow. We can work with this! Feel free to close this issue as by design. Thanks for all the assistance everyone!

@CyrusNajmabadi
Copy link
Member

Thanks for all the assistance everyone!

Definitely. Glad we could reach an acceptable resolution here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Resolution-By Design The behavior reported in the issue matches the current design
Projects
None yet
Development

No branches or pull requests

6 participants