-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Design issues around moving IDE code style analyzers to CodeStyle Analyzers NuGet package #38480
Comments
Can you leave me in the loop on this? |
Note: i question this:
I don't think the impl cost will be that high. Furthermore, i think there are several interesting potential approaches here that are feasible depending on the 'breaking change' bar we're willing to take. For example, i think we could just literally extract a ton of helpers into a separate library and just make things public. We'd then reference this library appropriately. This would also be beneficial to many customers who've been asking for the extensions to be able to use. We would, however, be explicit that you could use these helpers but that they might change at any time. We'd rev the version of this as we wanted, and it would be up to third parties to update appropriately. |
Use this as an upsell opportunity. What is devdivs goals here? Widespread use of this package? Or more use of the higher version of VS? |
Ick. That's pretty awful. What happens when we want to change something that needs an update on the analyzer and fixer? We go through a multi-month process of fixing up the analyzer, then waiting till we can reference it, then updating the fixer to support the changes in the analyzer? It just seems hellish in temrs of supporting things. Imagine if we had to do the same for adopting the compiler APIs and fixes. Yes, the fixers use a lot of internal stuff. But then we can address that. We move a fix at a time, moving the helpers that they need. THe vast majority of helpers are totally trivial to expose in a reasonable way. i.e. they're just extension methods, or helper services for answering questions about the language, or helpers for constructing/manipulating code. They're fairly well self-contained and would not be difficult to make available. |
Yes. Please please please avoid this. It's really awful how messy some layers of the IDE have already gotten in hte last couple of years. There is just blatent layering violations everywhere. Some features just hardcode knowledge in of other features. There always seems to eb time to do things in a hacky fashion, but never time to properly do things, even if doing that would make efforts like this much simpler. |
@CyrusNajmabadi Let me try to summarize your opinion here, please correct me if I am wrong:
This seems like a reasonable approach as well, though I see a few difficulties upfront:
We can certainly bring this up as another potential approach to tackle this work. We will keep this issue updated with any design decisions/updates before we start any concrete porting work. Note that my PR #38481 was just a quick prototype to gauge the required work and issues. |
Can we not just move these into the CodeStyle layer? And then just ship the CodeStyle stuff in Roslyn itself? i.e. the way i see it is that we have "features" today which is a bit too big. It's an aggregation of a ton of stuff, with analyzers/fixers being a large (but not whole) portion of it. So we're just breaking out the analyzer/fixer portion of 'features' into its own layer. |
It's really unfortunate to be constantly held hostage by RPS and to basically be expected to hack thnigs together to work around that problem. Factoring things into packages/layers is sensible and keeps it possible to efficiently keep delivering updates in a healthy way without building up debt. IIRC, if you look at something like .net core, the expectation is that a core app will end up referencing >100 dlls (with several of them going well into the multi-hundreds). Not being able to just break out a large, but cohesive, grouping of code when sensible, is very limiting. RPS should be an approach to not accidentally regress perf. It shouldn't be used to disallow changes completely when they're sensible and appropriate. |
Say that the entire surface area of this DLL is weakly supported. If you want, put everything under a |
If we can pull the analyzers/fixers out into the new layer, then i would recommend doing the same for tests. But i don't care a huge amount. The tests today are already in the wrong place (i.e. they're in the EditorFeatures test library, instead of the Features test lib), so i don't care much if they keep staying in the wrong place. |
Overall the summary is mostly what i think. I've clarified some of my position above. Basically, i'm hugely opposed to continually hacking away at this stuff. This is core critical functionality that the Roslyn team owns that should be well designed and implemented in a clean, cohesive, and healthy manner. The latter bit is especially important. I've given feedback several times over the last couple of years how we just keep having feature goals that we're not willing to properly assign resources to. So we have cut corners all over the place and the entire codebase it just building up more and more debt. This debt doesn't seem to be acknowledged**, but it continues to cost and harm future development. Unfortunately, this then gets ignored with the next big piece of work (like LiveShare) where more debt is piled on. I do not believe this approach is sustainable and I worry that the attitude from the top is somewhat of a "not my problem" take on things. i.e. the debt will keep piling up until it all has to be burned down. Instead, of spending the proper amount of tiem to do things right so that the same Roslyn we have now can be used 10+ years from now effectively. -- ** Specifically, the approach seems to be "we'll file bug, but then do nothing about it". Having the bug isn't the problem that needs addressing. It's the actual technical debt that is the problem. |
Another way to look at it: we have parts of roslyn that are effectively so complex and difficult to work with that they ahve not received more than tiny updates in them in around 5years now. That's because the debt piled up so fast, and the people working on those parts moved on, and now no one can actually handle updating things in those areas. We've basically made it so that the cost of getting out of that debt is higher than any value provided in improving those feature areas. That's really bad, and shows us how easy it is to get to that point. I would like to keep Analyzers/Fixers from descending into that. We need to stay in the position where it's simple and easy to author these, test these, update them for new use cases (or user reported issues), all while making it simple and easy for all of them to utilize and benefit from the shared infrastructure underneath. |
That would be my most preferred approach as well. However, note that we tried the exact same strategy for creating separate External Access projects/assemblies for each of our internal partners that had IVTs for Roslyn IDE layers, and this was rejected outright by RPS team due to additional assembly loads - this was the primary reason I did not mention this approach in the original description. We can probably pitch this refactoring as a requirement for editorconfig based CI support for IDE analyzers, and that might take it through, but we will be surely asked about potential alternatives that do not involve additional assembly loads. |
We can merge assemblies together like the editor team does if it really matters. I would personally like us to have a more enforceable way to keep things separate |
That would be great! AFAICT, roslyn should really only have '3 dlls' at the end of the day (through the merging of many layers). The core, language-agnostic, layer. All the way from teh compiler layer up through the VS layer. Then a dll for C# and a dll for VB. Those are the 'vertical slices' that you'll always have in VS, and it makes sense to deploy like that to avoid the RPS penalty. |
🔗 Related to #18818 |
This is covered by #18818. Only one analyzer should run for a given diagnostic ID. |
The design for this is complete in #18818, but it still needs to be implemented to match the feature specification. |
@sharwell That will be true only if we take @CyrusNajmabadi's suggestion and have only one assembly with the analyzer/fixer implementaton. If analyzers are in different named packages (CodeStyle versus Features), there can be no automatic de-duping even if #18818 is implemented. |
@mavasani #18818 defines and then explicitly states that the assembly name and identity is unrelated to the process of selecting diagnostic analyzers to run.
|
@sharwell I believe the above design requirement in #18818 is flawed. We cannot de-dupe and exclude analyzers from different analyzer assemblies based on just the fact that they happened to use the same diagnostic ID. I don't believe that issue has been brought for design discussion yet. |
ProposalUser Experience
Packaging
IMO, achieving the above packaging and user experience story for all of our analyzers requires the following from implementation perspective:
ImplementationAchieving the above packaging and user experience in a single shot is going to be challenging. So, I propose dividing it into stages as follows:
Stage 1
NOTE: We will also need corresponding C# and VB counterparts for all of above shared projects for language specific stuff.
Stage 2
Stage 3
Stage 4
|
@sharwell @mikadumont @vatsalyaagrawal I would like to bring up the new proposal from #38480 (comment) for discussion in today's design meeting. |
@mavasani Could you list the IDE analyzers we thing we should move to a package? |
Ideally, almost all of them (except probably for things like EnC, RenameTracking, etc. which are pure IDE-only ones). I think we should start with IDE analyzers that do not have any workspace dependency. For example, I prototyped one of them in #38481 and moving the analyzer and code fix into CodeStyle package itself was quite straightforward. I believe once we have all the utilities and helpers/extensions sorted out, majority of analyzers should be easy to move and start providing value add to customers. |
We're going to need to list them individually. I'm concerned that many would not meet the expected bar for avoiding false positives in NuGet-installed analyzers. |
@sharwell If any of the IDE analyzers are causing false positives, they should not be enabled in the IDE even today. I think we should move all (possible) IDE analyzers to NuGet package and decide to disable them by default if we think they are causing false positives. Enabled ones will still be suggestions or hidden diagnostics by default, so they cannot degrade the end user experience unless they explicitly bump up the severity of any diagnostic to be warning/error. Unless of course user is bulk configuring all |
All the analyzers in CodeStyle NuGet package are suggestions or hidden diagnostics by default. Hence, executing them in CI by default is an unnecessary performance overhead during build. This is especially critical when we attempt to move the CodeStyle NuGet package into the SDK as outlined in the User Experience [here](dotnet#38480 (comment)). This change auto-generates and embeds a .props file into the CodeStyle NuGet package that disables all analyzers in the package by default when executing command line builds. Current approach generates a `NoWarn`, but once dotnet#42219 is implemented, we will replace it with a global/base editorconfig which can ship with the NuGet package.
The design issues here have been resolved, and we have ported nearly half of the IDE analyzers to a pre-release CodeStyle NuGet package. Closing this issue. |
As part of #34907, we want to move as many IDE analyzers as possible into the CodeStyle analyzers NuGet package so the rules can be enforced on build/CI. We have already moved IDE0055 (Formatting analyzer) to this NuGet package, and I tried to move an additional analyzer to the package (#38481), but ran into bunch of design questions/issues that need to be discussed.
Diagnostic Analyzer: This is the core piece that needs to be ported from IDE Features project to CodeStyle project. Design questions in this effort:
Design Proposal: "yes", otherwise we run into issue about identical diagnostics with just a differing ID for each violation, duplicate suppressions, duplicate configuration (editorconfig/ruleset/nowarn entries) etc.
Personally, I hate the reflection based approach and would strongly prefer just moving to Microsoft.CodeAnalysis 3.x.
Code fixer: This is likely the most controversial and tricky piece to port, given that majority of our code fixers depend on internal IDE workspace and language services and extension methods, which in turn use bunch of internal utilities, helper utilities (for example, symbol equivalence comparer), etc.
Should we port IDE code fixers to Code Style layer? Theoretically speaking, this is not required as code fixers never execute in CI. Following are the pros and cons of this porting effort:
Design Proposal: Given the extremely large number of highly impactful CONS and the overall implementation and maintenance cost likely being a deal breaker for this effort, I would like to propose that we don't port the code fixers to code style layer. We should harden the code fixers so they gracefully bail out if the diagnostic location/properties is not as it expects, so it can handle analyzer breaking changes gracefully, and we can recommend customers to just upgrade to latest analyzer NuGet package to light-up the IDE code fixers again.
Unit tests:
Overall Design Proposal: I think we should try to keep a very good balance between ideal and practical aspects of the above approaches. I fear taking the ideal approach, which would be to port analyzer, code fixers and unit tests (also moving them to new analyzer/fixer test library) would make this effort almost impossible to complete.
IMO, we should do the minimal implementation/maintenance cost approach which would be feasible and also gives end user value add: port just the diagnostic analyzers and do not port the code fixers or unit tests, but just ensure that the code fixers are hardened against analyzer breaking changes and tweak existing IDE unit tests to also run with ported CodeStyle analyzers.
The text was updated successfully, but these errors were encountered: