Remove special casing code for F# in diagnostics infrastructure. #31243
Comments
|
Tagging @jasonmalinowski @jinujoseph I would like resolution on this PR before #31134 goes in. If there are no other choices, and #31134 is the only feasible way to proceed, that's ok. But IMO, we have enough hacks and problems in Roslyn to continue adding more. We should be pushing for people to do things the right way versus introducing more and more special cases for expediency. |
|
just to make sure people don't get wrong idea due to wording used. or Cyrus's opinion on what is hack and what is not. in F# script file support PR, there is no mention of F# specific code or a hack in any place. the PR contains refactoring to share more code between default diagnostic analyzer service that is used in misc workspace, and Host workspace (VS Workspace). now 2 basically use the same logic to compute diagnostics for given analyzers. except for all host (VSworkspace) specific optimization such as caching, persistence or OOP usage. that is general code clean up by sharing more code between 2 diagnostic analyzer services. whether we even merge 2 to 1 is a different discussion. as I said in the meeting, I do not want to make it like tagger which becomes 1 monolithic code base where no one can figure out what will change when one changes one line of code. another thing the PR is trying to do is attempt to provide common implementation for script file that opened without any project context. when FSharp file is opened in a project context, existing FSharp code takes care of that. it is only when the script file is opened without any project context. which is specifically how misc workspace is designed to kick in. now the misc workspace enables semantic errors for script file by default. so one can get semantic errors by default. again, this is generic thing. any script file (SourceCodeKind.Script) that participates in our misc workspace will get this behavior by default. this is not sepcific to FSharp. but any document inserted to misc workspace with SourceCodeKind.Script. |
The feature exists solely to support F#. There are no customers asking or needing this. Hence, it's F# specific. It is also a "hack" because it reimplements logic htat already works through teh supported roslyn language plugin mechanisms. Adding additional logic and complexity to support something already supported is realistically a "hack". It's also a "hack" because it doesn't support the full customer ask. For example:
The change doesn't even fully implement the existing behavior that is waht is being asked for. Because of that, there were suggestions to do:
When you have to continually add more and more features just to get behavior that is already supported, that is definitely very hacky. You'll note that TypeScript doesn't need this sort of behavior. Things light up properly because TS isn't sidestepping the expected way to integrate with Roslyn, and then also asking to get the same behavior for TS that the other languages get. |
sure. I get that is your definition of F# specific. that is not mine, though. since, now when other team came to us like razor/xaml (by the way, I dont get what you are saying about these, they do open in misc as well when opened without project context, they do not create vitual project or something you mentioned), we can simply say, when you participate in misc workspace, give right info to workspace what is your script file extension is, then it will light up automatically without us doing anything. that's in my definition, it is not F# specific. |
A 'project' is fundamentally a language construct. All it means is "a grouping of files i want to be considered connected". It doesn't mean "you have to actually have a project file". For example, TypeScript has teh concept of "projects" without any project files at all. In fact, how TS does it is exactly like how F# wants to do things. Typescript can just do
This functionality is already supported with more capabilities with the normal Workspace. For example, in the normal workspace (which is waht every other language uses), you automatically get things like reanalysis of other documents when you edit one of your documents. There's no need to special case anything here. |
|
sure, you can discuss your opinion. for now, I don't agree that is right or better solution. I get that you think that is better. sure we can discuss and get other people's thoughts as well. but, asking each team to create thier own thing for a file opened out of project context is in my opinion, not better. it would be better we provide one that it just work for most of things. I dont know what that is. but we have misc workspace right now, so, until we figure out what that ultimate best thing is, not doing anything is worst in my opinion. |
What other customer is asking for this?
Razor opens files in the misc workspace? How do they get semantic errors in that case then today?
You are describing how the normal workspace works. As your PR clearly shows, it does not light up automatically. You have to do work to try to make these workspaces behave the same. And even in your PR the workspaces still don't behave hte same. That's why you had to outline several other mechanisms that would have to be added just to do this. Again, this was considered when doing TypeScript. We could have make it so that TS just opened in the misc workpsace, and then had all our features updated to support that scenario with equal fidelity to the normal workspace. But this was rightly rejected as it would have meant a large burden on Roslyn to support all these scnearios, as well as a large testing burden on Roslyn to ensure that it was always maintaining and keeping up to date these alternative ways for features to work that paralleled how normal C#/VB work. |
|
okay. things in go in circle again. I will stop here. I didn't say razor got the script semantic errors. I just said, it doesn't do what you are saying. just open cshtml/xaml in VS without any project and see where it gets opened. anyway, we will discuss your approach in next design meeting or FSharp. and see whether we are event want to do it, or want to do what you are saying, or have some other approach. |
are you talking about C#/VB script files? they are not since they are specifically marked not to open in misc workspace. |
Why didn't we just do that for TypeScript/JavaScript then? Why does Razor plug into our normal Workspace?
To me, that would be the normal workspace. It's extensively well tested. It supports literally the entire Roslyn featureset. It is not a hack. It has a well defined way for languages to plug in. It gives consistent behavior across languages. It doesn't require continually adding more knobs to try to get things to behave consistently. Not only that, but it's a tried and tested mechanism. We not only ensured it works great for project-file-based languages, but we also ensured it works great for file/folder oriented languages.
Each team doesn't need to create their own thing. For example, TypeScript already invested in a usable system for defining projects from files/folders. It would be great to think about ways to adopt and share that code to enable other file/folder-oriented scenarios to work better. |
|
Razor opened in a project context or xaml plugs into our normal workspace, not one that opened outside of a project context. my PR is also for FSharp script file opened outside of a project context. one that is opened within project context, does do what you are saying and it has nothing to do with my change. that works already and works fine. |
Great. Can you please hold off on merging the other PR on in before that. We shoudl not be putting in unnecessary code when there are cleaner and healthier ways to implement these requests. If F# doesn't have the resouces to integrate properly, then this PR can be taken in as an unfortunate unblocker for business purposes. We have ample precedence for that in Roslyn. However, until there is an actual customer ask that requires this and has no better alternative, we should not be taking in these sorts of PRs. it jsut adds more and more hacks to the workspace, which is already complex enough and hard enough to maintain. |
Yes. I realize that. I'm pointing out that this is a hacky way to support that scenario. TS/JS open files "outside of a project-file context" all the time, but don't need this sort of support. Because, as i mentioned in #31243 (comment), this is totally feasible to support without needing special logic within Roslyn. |
|
no. I dont believe the result of that meeting will change anything on the PR. like I said, the code is already there, it already make the experience better. so, no point on holding it until we get something else which we don't know when we will have. and once we have something else, we can just change how things work. whether it is the way you are doing or something else. |
|
ya, you already repeated several times that you think what ts you decide to do is best way and better way. and I already said I am not sure. so let's not repeat and hear other people's opinion. and I will make sure this get discussed in next design meeting. |
Right. So all the mainline language cases work with the normal workspace.
All thse cases work with the normal workspace just fine. The only thing missing from that list is "Fsx script without project-file". There is no need to create special cases in the core Roslyn code to support that when there are tried and tested approaches that work already. We've already proven them out with TS. |
|
tagging @jinujoseph and @sharwell for design meeting item. |
|
oh my got. you just keep changing my wording.
only if it is opened under a projec context. without it, it doesn't get into VS Workspace. |
That's not a good enough reason. That has to be weighed against things like hte impact on code quality. The additional maintenance and test burden. We don't allow any PR to jsut go in just because it can make a partner's life better. For example, there have been many requests over hte years from partners (including PRs) that we've rejected for being the wrong way to do things. PRs should go in when the team feels like it's the right solution to the problem. Not just because it makes an experience better.
Yes, tehre is a point. Consider just a simple thought experiment. Two people create PRs that make an experience better. But one is architected poorly and one is architected well. Would you take the former? Of course not. Because we have to consider the right way to solve problems and we should be taking tha tinto consideration when reviewing any PR. This happens all the time, and it's a prime responsibility of the team and those reviewing PRs. This is not happening in a vacuum. |
Yes. That was my point. It's what i've been saying since hte first post. That's the issue entirely. Stop opening it without a project-context, like all the rest of the scenarios. |
I have not changed any of your words. I have quoted you directly each time. |
|
you cut the word out of it. I can cut any part of your wording and change meaning. again, I told you that I will tell your opinions. |
I had no doubt about that part. :) As you can see, i've never contradicted anything you've said about that. :) |
This is another sign that this is a hack made for a single customer. We both don't even have tests, and the only way to ensure that something doesn't break is to manually test on that teams' end. If we're exposing this through our API as a supported way for people to plug in, we should be able to test things properly. We can test the rest of our workspace layers and the analyzer subsystem fairly narrowly. |
Either this is a real workspace feature, in which case we shoudl be testing it (like we do the other components that our other partner languages plug into), or it's not real workspace feature and it's just a workaround for a partner. The latter is fine. But that shows what the PR is truly intended for. It's not a general-purpose architectural change to support something for lots of potential consumers. It's a one off that we want to put minimal work into, and we don't even want to test adequately ourselves. this sort of approach pushes me even further away from wanting to accept it. IMO, anything going in at this level of the workspace, and this level of the analyzer subsystem should be adequately tested. |
|
well. we have many of those areas that we don't have cover for partner cases including razor, xaml, typescript, F#, winforms, liveshare, pythia, code lens, fxcop and more. otherwise, we won't have broken partners as we do refactorings or add new features. but we do not call those hacks. but sure you can call that hacks if you want to. |
jinujoseph
added
Area-IDE
Need Design Review
|
Marking this as design review. Team will discuss and come up with a decision. |
|
I made the same code work for C# and VB as well, so it is no longer only F# uses new capabilities. but C# and VB as well. |
|
Closing out. We want everyone to move to LSP. |
@heejaechang asked me to open this.
#31134 Seems to be introducing an inappropriate mechanism to support standalone files. As mentioned in the PR, core parts of the customer-as are not supported. For example: #31134 (comment)
Instead, this acts as a half measure, making it appear as if a file works properly, but actually does not. This is because the approach works to try to make the misc-workspace work somewhat like a real workspace. A more appropriate fix is to just have F# team create a real Project that is added to the normal Roslyn workspace. With this approach, every single feature works properly as the real workspace is how everything is built today. For example, diagnostics would work properly for all scenarios, not just the single-file case.
Note: this is the approach we've taken for C#, VB, TypeScript, JavaScript and I believe Razor/Xaml. This is the most well tested system, and it has been tested extensively to ensure appropriate behavior across any language that wants to plug in. The misc-workspace exists simply as a historical fallback. It is not feature-rich, and it is not very well tested. It also increases the burden for maintenance as any partner request to work in that means duplicating logic and testing already available in the normal workspace. For example, if F# wants analyzer behavior similar to the normal workspace, that will have then be made to work on top of the existing PR that is trying to go in.
It is especially telling that there is no test coverage at all for these new codepaths, and nothing that verifies taht it actually works properly. This is the problem of creating one-off solutions for use cases already handled by the normal system. Not only is there the burden of the extra code to maintain for a single customer, but there is also the need to have to write a whole set of tests to validate that things work properly. At least when using the existing infrastructure, testing is at least covered just by having our normal languages all sharing the same codepaths.
This was an approach we looked at for TypeScript and rejected because it simply would lead to so much extra unnecessary code in Roslyn (including having to have a much larger test burden that Roslyn clearly would not want to have to support). Instead, we went the approach of ensuring that TS followed the normal Roslyn workspace model, ensuring that all the same code was runnign for all the languages, instead of having two separate systems working together that need to be kept in sync.
The text was updated successfully, but these errors were encountered: