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

Add C# code fix/refactor for new set of 'minimal' API surface areas #56283

Closed
timheuer opened this issue Sep 9, 2021 · 23 comments
Closed

Add C# code fix/refactor for new set of 'minimal' API surface areas #56283

timheuer opened this issue Sep 9, 2021 · 23 comments

Comments

@timheuer
Copy link
Member

timheuer commented Sep 9, 2021

Brief description:

In .NET 6 and C# 10, several new defaults of glorious language enhancements were made and new experiences in tools make use of them namely in the new project/item template scenarios e.g., dotnet new console

The new .NET 6 console output produces an app that makes use of the following new tools/language features:

  • top-level statements (no namespace/class definitions)
  • implicit usings (<ImplicitUsings>enable</ImplicitUsings>)
  • nullable checks (<Nullable>enable</Nullable>)

Customers may want to refactor back to older variants (or also older apps that can make use of these capabilities may want to refactor TO this pattern. A code fix created that would basically toggle usage of this when the pattern is able to be recognized.

Also having something like this that EditorConfig would support as well would be fantastic.

Languages applicable:
C#

Code example that the analyzer should report:
a single .cs file that has only this content (no namespaces, no 'main')

Console.WriteLine("Hello, World!");
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Sep 9, 2021
@KathleenDollard
Copy link

Can I clarify this?

  • Are you suggesting (via the word toggle) that users could go back and forth between the new style and the old style?
  • Or, are you suggesting (via the capitalized use of "TO") that the user only be able to go to the new platform?

Also, do we really want this to be a single concern, rather than individual fixers (some of which I think we have already)? I do not know whether we have other places where we have an aggregate fixer. If not, I think these being set individually is more important than being set together - at least in existing code.

@CyrusNajmabadi
Copy link
Member

In general we do not provide features to move back to the older styles of doing things. We instead just go in the forward direction.

Note: implicit usings are neither a c# nor Roslyn thing. They are part of the .net sdk.

@timheuer
Copy link
Member Author

timheuer commented Sep 9, 2021

  • Are you suggesting (via the word toggle) that users could go back and forth between the new style and the old style?
  • Or, are you suggesting (via the capitalized use of "TO") that the user only be able to go to the new platform?

Dunno, worthy of a discussion. We certainly have fixers that 'toggle' (use implicit/explicit type), so I could see that for maybe top-level statements (although once you remove the namespace, what's the 'back' 2 days later when the context is lost).

Also, do we really want this to be a single concern, rather than individual fixers (some of which I think we have already)? I do not know whether we have other places where we have an aggregate fixer. If not, I think these being set individually is more important than being set together - at least in existing code.

We do have some aggregate, but not a lot. This is also a good discussion point on design. I definitely could see ImplicitUsings being one that stands on it's own, as does top-level program.

@timheuer
Copy link
Member Author

timheuer commented Sep 9, 2021

In general we do not provide features to move back to the older styles of doing things. We instead just go in the forward direction.

Exhibit A: :-)

2021-09-09_08-16-21

Note: implicit usings are neither a c# nor Roslyn thing. They are part of the .net sdk.

Right, sorry if I suggested it was...noting these are '.NET 6 and C# 10' things in my list here. Are you suggesting that changes things though as we have fixers for non-Roslyn/language things

@Youssef1313
Copy link
Member

In general we do not provide features to move back to the older styles of doing things.

Is file-scoped an exception to this? IIRC, the refactoring and code-style option works both ways. Why this would be different?

@CyrusNajmabadi
Copy link
Member

Hi Tim. First, can you fix both your posts? You're not quoting properly, making out very difficult to read.

Second, as I said, "in general" :-)

Note that var has bidirectionality because it's a matter of clarity. That isn't a thing for cases like nullable, or top level statements.

Third:

Are you suggesting that changes things though as we have fixers for non-Roslyn/language things

I'm basically saying that anything related to implicit usings should likely be handled by the sdk team. They're not part of either c# or Roslyn.

@CyrusNajmabadi
Copy link
Member

is file-scoped an exception to this?

Yes. Because there's a functionality difference. For example, you cannot have multiple namespaces when using top level namespaces. So there's a way to switch.

That's the same reason that we make it easy to switch from an expression bodied member to a block. It's not to 'move back', it's because you need that if you're going to have multiple statements.

@timheuer
Copy link
Member Author

timheuer commented Sep 9, 2021

I'm basically saying that anything related to implicit usings should likely be handled by the sdk team. They're not part of either c# or Roslyn.

I think this warrants a discussion when a design happens here. Want to make sure we have the best customer experience if implemented by the experts in this area that know these type of experiences.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Sep 9, 2021

I think this warrants a discussion when a design happens here.

I'm not sure what you mean. Implicit usings weren't designed here :) They're literally not a part of the language or Roslyn. No code exists in the compiler or IDE related to them at all, and they're not a C# language constructor (v10 or otherwise). They're entirely an SDK construct. Can you clarify what you'd like Roslyn to do here wrt them?

--

Note: you may be thinking about 'global usings' (which are not the same as 'implicit usings'). The former is part of the language and is supported in the Roslyn Compiler and IDE. However, i'm unaware of any new feature work we intend to do there for that feature except maybe have something that detects if all your files use a particular using, and possibly suggest to you that you might want to make it into a global using.

@KathleenDollard
Copy link

We have three things:

  1. File scoped namespaces: Due to functionality differences, there are fixers to go both ways if I understand this thread (I haven't played with that yet.)

  2. Top level statements: I see this as also being either a functionality question, or almost a functionality question if you develop a need to access your implied main method, such as for tests. I can't recall if we made them both speakable, but if we did, I could understand an argument that if they are accessed from the outside a programmer should be able to set up for people to see where the main appears - such as allowing people to search and find where that "main" is.

  3. Implicit usings: Yes, the SDK provides the implementation - but the control of the opt-out is in the project file. Skipping over the org chart questions, this does not seem like a simple problem. Unless we were to accept broken code after the change, opting out would not be enough. We would also have to add in the right using statements either in individual files or a project scoped global usings file. I don't know enough to know if that would be as gnarly as it sounds.

@jinujoseph jinujoseph added Investigation Required and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 10, 2021
@jinujoseph jinujoseph added the Need Design Review The end user experience design needs to be reviewed and approved. label Sep 10, 2021
@jinujoseph jinujoseph added this to the Backlog milestone Sep 10, 2021
@CyrusNajmabadi
Copy link
Member

Skipping over the org chart questions

Note: this wasn't really about shipping the org chart. It was genuinely that literally all the code for this exists outside roslyn (and C# for that matter). It's an entirely a new concept created by the sdk, and i think all the code for it should live there :)

File scoped namespaces: Due to functionality differences, there are fixers to go both ways if I understand this thread (I haven't played with that yet.)

Yes, that's correct :)

Top level statements: I see this as also being either a functionality question, or almost a functionality question if you develop a need to access your implied main method, such as for tests. I can't recall if we made them both speakable, but if we did, I could understand an argument that if they are accessed from the outside a programmer should be able to set up for people to see where the main appears - such as allowing people to search and find where that "main" is.

I feel like the use case of "directly calling into main" is such a narrow case that we don't really need a special feature to handle it. If a user finds themselves in that position... well, it's just a small matter of adding class Program { static void Main(string[] args) { ... } } around their code.

@sharwell
Copy link
Member

@timheuer It sounds like your original request is for a set of refactorings which I've come to call "language migration aids". Here are some previous cases where I've filed related issues:

DotNetAnalyzers/Proposals#19
DotNetAnalyzers/Proposals#20

As it applies to this issue, these refactorings would be designed to automate the transition of C# X to C# Y, where Y<X. It would be especially useful for cases where you need to retarget a project to an earlier framework, e.g. from net6.0 (implicitly C# 10) to net5.0 (implicitly C# 9).

Would you agree this is a correct interpretation of your request above?

Assuming I have correctly interpreted your request, my next questions is: is it possible to modify (add arguments to) the dotnet new console command to create a project targeting net5.0 (and only using C# 9 features)? Perhaps this would eliminate the need to make several style changes right after the project is created?

@timheuer
Copy link
Member Author

@timheuer It sounds like your original request is for a set of refactorings which I've come to call "language migration aids". Here are some previous cases where I've filed related issues:
...
Would you agree this is a correct interpretation of your request above?

Kinda? I still think it can be more of just a migration but as your code may shrink/grow it is a pattern that a developer may want to have. Or a team may want to enable (editorconfig) as a pattern for certain modules as a coding style. This came out of a discussion with @davidfowl as we were re-reviewing the template work specifically for Console a more bare-bones example.

Assuming I have correctly interpreted your request, my next questions is: is it possible to modify (add arguments to) the dotnet new console command to create a project targeting net5.0 (and only using C# 9 features)? Perhaps this would eliminate the need to make several style changes right after the project is created?

This was discussed but we are not going to do that right now.

@jmarolf
Copy link
Contributor

jmarolf commented Sep 13, 2021

Customers may want to refactor back to older variants (or also older apps that can make use of these capabilities may want to refactor TO this pattern. A code fix created that would basically toggle usage of this when the pattern is able to be recognized.

I would be fine with a set of refactorings that moved people towards these defaults, but I do not think we want to make it easier for folks to move away from them.

Also having something like this that EditorConfig would support as well would be fantastic.

I don't really think an editorconfig option for these is appropriate. This isn't a stylistic concern; this is how we want people to write code going forward. The one case where we've done this is the past is var because it was so contentious, but there are reasonable arguments to be made for not using var in some code baseses.

Looking at each feature:

top-level statements (no namespace/class definitions)

The .NET community does not need to waste time thinking about entry points anymore and I view that as a net positive. There is nothing you cannot do in a top-level statement that would indicate we need a refactoring here. The other refactorings we offer (linq to foreach, etc.) that transform between different language forms do so because there is a real difference between them semantically. There isn't one for top-level statements and we don't really want people to go back.

implicit usings (<ImplicitUsings>enable</ImplicitUsings>)

As Cyrus has said there are engineering problems that would need to be solved here. The SDK would need to give use some knob we could turn here. But putting that discussion to the side this is the request I feel is most reasonable as a setting in the .NET CLI / Visual Studio. I do not think a refactoring is the correct design for this functionality.

nullable checks (<Nullable>enable</Nullable>)

The goal is to get more people to have this feature on not less. I see no customer advantage to making it easier to opt out of more type safety.

@jasonmalinowski
Copy link
Member

We agreed on the next steps for each of the features:

  • For top level statements, let's not do anything until we hear more feedback. There was general agreement on needing to make it clear for users to understand how things changed, but we didn't really know how best to help.
  • For implicit usings, we should reach out to the project system and see if there's good support we can add to the project property pages similar to what is done for VB today.
  • For nullable, we should defer to Fixer to add project-level nullable enable option #52893 and continue to follow along that; we didn't feel having a "go back" was terribly helpful.

@jasonmalinowski
Copy link
Member

And generally, for the question of "should we have refactorings that go both ways?" a heuristic was proposed: do we expect a modern application to have a mix of the new feature and old? One example was given of expression-bodied members: you can't use them in all places, and so even a fully modern application will still have a mix. But for something like ?., we don't have a go back -- there's just no reason to write the old pattern, and even a naïve conversion still would cause issues.

@jasonmalinowski
Copy link
Member

@sharwell: do we still need to keep this bug open tracking any specific piece of work at this point? If we do, I admit it's not entirely clear what it is. 😄

@jmarolf
Copy link
Contributor

jmarolf commented Sep 13, 2021

filed dotnet/project-system#7586 to track the project system side of things

@drewnoakes
Copy link
Member

Project System is tracking this in dotnet/project-system#7488.

@drewnoakes
Copy link
Member

But for something like ?., we don't have a go back -- there's just no reason to write the old pattern, and even a naïve conversion still would cause issues.

Consider though that unwinding a shorthand form is often just a stepping stone towards where you want to go. Using ?. as an example, there may be more code you wish to condition on the nullness of that value. Having a transformation from ?. to an if guarded statement would avoid a bunch of typing, copying and pasting.

I believe the same is true of other forms, such as ternary expressions, null coalescing operators, compound assignment operators, and even for some cases constructs such as foreach/using/lock.

In general it's useful to have suggestions where code can be expressed using higher level features. But sometimes when you're editing code you want to "lower" (to use compiler lingo) to a lower-level form, in order to transform your way from that point out.

@jasonmalinowski jasonmalinowski removed the Need Design Review The end user experience design needs to be reviewed and approved. label Sep 14, 2021
@svick
Copy link
Contributor

svick commented Sep 18, 2021

@drewnoakes How is the IDE going to know that you want to write an if statement, and not a ternary, or even something like a switch expression?

@drewnoakes
Copy link
Member

It wouldn't be a suggestion, just an optional transformation available via the lightbulb.

@CyrusNajmabadi
Copy link
Member

Closing out as we have not heard feedback since these features were introduced that indicate that these chnages are needed.

@CyrusNajmabadi CyrusNajmabadi closed this as not planned Won't fix, can't repro, duplicate, stale Oct 22, 2024
@dotnet dotnet locked and limited conversation to collaborators Oct 22, 2024
@CyrusNajmabadi CyrusNajmabadi moved this from Need Update to Complete in IDE: Design review Oct 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Complete
Development

No branches or pull requests

10 participants