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 fixers project and implement a fixer for FieldReferenceForObservablePropertyFieldAnalyzer #578

Merged
merged 6 commits into from
Jan 26, 2023

Conversation

333fred
Copy link
Contributor

@333fred 333fred commented Jan 22, 2023

This adds a fixer for MVVMTK0034, that simply swaps the field reference for what will be generated for the property. It leaves the qualification intact, basically just doing a text swap. Because it's a very very simple fixer, the built-in batch fixall provider works just fine and does not need special handling.

While I implemented the fixer and added some tests, I did not actually set up all the msbuild goop for properly packaging this as part of the existing packages (as, frankly, the multi-targeting and msbuild setup in this repo is very complicated and I didn't feel like trying to grok exactly how to add it given the setup). The fixer assembly needs to be put next to the analyzer assemblies. This PR will be editable by maintainers, so feel free to implement this in-place in the PR itself.

Fixes #577.

…blePropertyFieldAnalyzer

This adds a fixer for MVVMTK0034, that simply swaps the field reference for what will be generated for the property. It leaves the qualification intact, basically just doing a text swap. Because it's very a very simple fixer, the built-in batch fixall provider works just fine and does not need special handling.

While I implemented the fixer and added some tests, I did not actually set up all the msbuild goop for properly packaging this as part of the existing packages (as, frankly, the multi-targeting and msbuild setup in this repo is very complicated and I didn't feel like trying to grok exactly how to add it given the setup). The fixer assembly needs to be put next to the analyzer assemblies. This PR will be editable by maintainers, so feel free to implement this in-place in the PR itself.

Fixes #577.
@Sergio0694
Copy link
Member

Now for the fun part, making this thing actually work with multi-targeted packaging 🤣

@Sergio0694 Sergio0694 marked this pull request as ready for review January 23, 2023 00:14
@Sergio0694
Copy link
Member

FYI @Arlodotexe you might also want to take a look if you want to learn something new 😄
This still might also potentially be useful for you folks in Labs.

Copy link
Contributor Author

@333fred 333fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming you've tested the package in VS and seen the analyzers (and fix all) working?

@Sergio0694
Copy link
Member

Not yet. I've just marked this as ready to review as it's not clearly still work in progress, is all 😄

@Sergio0694
Copy link
Member

Mmmh trying this out, I get this:

System.InvalidOperationException : The solution does not contain the specified document.
   at Microsoft.CodeAnalysis.Solution.CheckContainsDocument(DocumentId documentId)
   at Microsoft.CodeAnalysis.Solution.WithDocumentSyntaxRoot(DocumentId documentId,SyntaxNode root,PreservationMode mode)
   at async Microsoft.CodeAnalysis.IntroduceVariable.AbstractIntroduceParameterService`4.IntroduceParameterAsync[TExpressionSyntax,TInvocationExpressionSyntax,TObjectCreationExpressionSyntax,TIdentifierNameSyntax](<Unknown Parameters>)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at async Microsoft.CodeAnalysis.CodeActions.CodeAction.ComputeOperationsAsync(<Unknown Parameters>)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at async Microsoft.CodeAnalysis.CodeActions.CodeAction.GetPreviewOperationsAsync(<Unknown Parameters>)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at async Microsoft.CodeAnalysis.Editor.Implementation.Suggestions.SuggestedAction.GetPreviewResultAsync(<Unknown Parameters>)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at async Microsoft.CodeAnalysis.Editor.Implementation.Suggestions.SuggestedActionWithNestedFlavors.PreviewChangesSuggestedAction.CreateAsync(<Unknown Parameters>)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at async Microsoft.CodeAnalysis.Editor.Implementation.Suggestions.SuggestedActionWithNestedFlavors.GetPreviewChangesFlavorAsync(<Unknown Parameters>)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at async Microsoft.CodeAnalysis.Editor.Implementation.Suggestions.SuggestedActionWithNestedFlavors.CreateAllFlavorsAsync(<Unknown Parameters>)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at async Microsoft.CodeAnalysis.Extensions.IExtensionManagerExtensions.PerformFunctionAsync[T](<Unknown Parameters>)

Kinda seems unrelated. Not sure why the code fix isn't showing up though. Let me try in a different project as well.

@Sergio0694
Copy link
Member

It works!! 🚀🚀🚀

Also validated batch fixer, works perfectly too. The fixer seems to be busted with a shared project, but that would seem to just be Roslyn choking up for whatever reason. It works perfectly fine in a standalone project, as in the picture above.

@Sergio0694
Copy link
Member

Hey @333fred are you able to run the unit tests locally? They're failing for me with some packing error, but they run fine in the CI, so I'm not entirely sure I understand what's going on with them. Can you try on your end as well as a sanity check?

@Youssef1313
Copy link
Contributor

Hey @333fred are you able to run the unit tests locally? They're failing for me with some packing error, but they run fine in the CI, so I'm not entirely sure I understand what's going on with them. Can you try on your end as well as a sanity check?

What is the error?

@Sergio0694
Copy link
Member

Message: 

Test method CommunityToolkit.Mvvm.SourceGenerators.Roslyn401.UnitTests.Test_FieldReferenceForObservablePropertyFieldCodeFixer.ExternalConditionalMemberAccess threw exception:
NuGet.Packaging.Core.PackagingException: The package is missing the required nuspec file. Path: C:\Users\sergiopedri\AppData\Local\Temp\test-packages\Microsoft.NETCore.App.Ref.6.0.0

Stack Trace: 

PackageFolderReader.GetNuspecFile()
PackageReaderBase.GetNuspec()
PackageReaderBase.get_NuspecReader()
PackageReaderBase.GetFrameworkItems()
PackageReaderBase.GetFrameworkItemsAsync(CancellationToken cancellationToken)
<ResolveCoreAsync>d__50.MoveNext() line 365
--- End of stack trace from previous location where exception was thrown ---
ExceptionDispatchInfo.Throw()
TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
TaskAwaiter`1.GetResult()
<44 more frames...>
TaskAwaiter.GetResult()
<RunAsync>d__62.MoveNext() line 181
--- End of stack trace from previous location where exception was thrown ---
ExceptionDispatchInfo.Throw()
TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
TaskAwaiter.GetResult()
<ExternalConditionalMemberAccess>d__3.MoveNext() line 284
--- End of stack trace from previous location where exception was thrown ---
ExceptionDispatchInfo.Throw()
TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)

I get a total of 114 failing unit tests across all source generator tests on all targets 🥲

@Youssef1313
Copy link
Contributor

@Sergio0694 Does deleting %TEMP%\test-packages help?

@Sergio0694
Copy link
Member

Oh, cool, that worked, thank you! 😄

Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you! :shipit:

Copy link
Contributor Author

@333fred 333fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's my pr so I can't mark it approved, but your changes lgtm

@Sergio0694 Sergio0694 merged commit f9ee156 into CommunityToolkit:main Jan 26, 2023
@Sergio0694
Copy link
Member

Sergio0694 commented Jan 26, 2023

Thank you @333fred! And @Youssef1313 for the review 😄

@333fred 333fred deleted the fixer branch February 19, 2023 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Code Fixer for MVVMTK0034 (do not reference backing field directly)
3 participants