[Refactoring] Switch to Roslyn CodeFixService by Therzok · Pull Request #3636 · mono/monodevelop

Therzok · 2018-01-12T13:59:05Z

No description provided.

mkrueger

I <3 removing code :)

Therzok · 2018-01-15T12:12:02Z

Seems like a file was not removed from the csproj.

Therzok · 2018-01-15T15:33:46Z

@mkrueger any idea why this would cause an unit test failure?

Therzok · 2018-01-16T14:18:23Z

This will conflict once #3668 is in.

Therzok · 2018-01-16T19:56:13Z

~~I'll refactor this PR and split stuff which doesn't directly affect code fixes into master.~~

Too much work to do that, I'll just rebase on top of newer editorFeatures once #3677 is in.

Therzok · 2018-01-18T18:11:03Z

Feedback from vaclav on the pick members UI. Will probably need to backport these fixes to the implement interface members UI.

Therzok · 2018-02-15T11:53:21Z

Some code actions depend on #3699 beign merged.

Therzok · 2018-02-15T14:18:24Z

Test failures unrelated.

(They were broken during editorFeatures dev)

mhutch · 2018-02-16T01:41:51Z

Looking at that "Pick members" UI, I'm curious how usable it is with the keyboard alone.

mhutch

Looks good. I had a bunch of comments but nothing blocking.

mhutch · 2018-02-16T01:42:49Z

...PackageManagement/MonoDevelop.PackageManagement.Refactoring/PackageCodeDiagnosticProvider.cs

-// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
-// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
-// THE SOFTWARE.
+////


Why's this disabled?

Not sure how to hook up nuget packages analyzers. They certainly don't belong in the host diagnostics provider

Yeah. I don't think they belong in the NuGet extension either...

Looking at
https://github.com/dotnet/roslyn/blob/48d978fbfc4a4437d3a5da60c4d823e2e593b491/src/VisualStudio/Core/SolutionExplorerShim/DiagnosticItem/CpsDiagnosticItemSource.cs, it looks like it just mirrors AnalyzerReference MSBuild items into AnalyzerReferences on the roslyn projects.

NuGet simply adds those MSBuild items into the project (SDK style projects will have this automatically, for classic projects we'll need to implement it). These items could also exist in a project file, or come from targets etc.

mhutch · 2018-02-16T01:51:45Z

main/src/addins/MonoDevelop.Refactoring/MonoDevelop.CodeActions/CodeActionEditorExtension.cs

 			currentSmartTag.ShowPopup += CurrentSmartTag_ShowPopup;
 			currentSmartTag.Tag = fixes;
-			currentSmartTag.IsVisible = fixes.CodeFixActions.Count > 0;
+			currentSmartTag.IsVisible = fixes.CodeFixActions.Sum (x => x.Fixes.Length) > 0;


Minor nitpick, wonder whether .Any(x=>x.Fixes.Length > 0) would be better as it short circuits

Yes, good point!

mhutch · 2018-02-16T01:54:09Z

main/src/addins/MonoDevelop.Refactoring/MonoDevelop.CodeIssues/CodeDiagnosticRunner.cs

-		static IEnumerable<CodeDiagnosticDescriptor> diagnostics;
-		static Dictionary<string, CodeDiagnosticDescriptor> diagnosticTable;
-		static SemaphoreSlim diagnosticLock = new SemaphoreSlim (1, 1);
+		static MonoDevelopWorkspaceDiagnosticAnalyzerProviderService.OptionsTable options =


Given there are so many of these, maybe move to a single MonoDevelopWorkspaceDiagnosticAnalyzerProviderService.Instance member?

I made many copies because it's a long name. :p

mhutch · 2018-02-16T01:55:32Z

main/src/addins/MonoDevelop.Refactoring/MonoDevelop.Refactoring.addin.xml

 		<Assembly file="RefactoringEssentials.dll"/>
 	</Extension>

+	<ExtensionPoint path="/MonoDevelop/Refactoring/AnalyzerAssemblies">


Awesome, extensions will finally be able to cleanly add analyzers 🎉 🎉 🎉

mhutch · 2018-02-16T01:57:25Z

main/src/addins/MonoDevelop.Refactoring/MonoDevelop.Refactoring/AnalyzeWholeSolutionHandler.cs

 			var providers = new List<DiagnosticAnalyzer> ();
 			var alreadyAdded = new HashSet<Type> ();
-			var	diagnostics = await CodeRefactoringService.GetCodeDiagnosticsAsync (new EmptyDocumentContext (project), LanguageNames.CSharp, default (CancellationToken));
+			var diagnostics = options.AllDiagnostics.Where (x => x.Languages.Contains (LanguageNames.CSharp));


This pattern seems to come up a bit, maybe a options.GetDiagnostics(string language) would be better

Also, wondering whether we can stop hardcoding C# everywhere?

When we have a language mapping in the project model, sure.

mhutch · 2018-02-16T02:00:39Z

...efactoring/MonoDevelop.AnalysisCore/MonoDevelopWorkspaceDiagnosticAnalyzerProviderService.cs


+		void LoadAnalyzerAssemblies()
+		{
+			RuntimeEnabledAssemblies = AddinManager.GetExtensionNodes<AssemblyExtensionNode> (extensionPath).Select (b => b.FileName).ToArray ();


Would be nice to subscribe to extension changes so analyzers from newly installed extensions can load without a restart.

Roslyn doesn't re-query that. It's not a workspace service

Oh, that's a shame :(

I guess we could always add them as workspace AnalyzerReferences if we wanted to support this. Probably safest just to add a forced restart after installing or uninstalling extensions though.

mhutch · 2018-02-16T02:04:33Z

...noDevelop.AnalysisCore/MonoDevelopWorkspaceDiagnosticAnalyzerProviderService.OptionsTable.cs

+	{
+		internal class OptionsTable
+		{
+			Dictionary<string, CodeDiagnosticDescriptor> diagnosticTable = new Dictionary<string, CodeDiagnosticDescriptor> ();


It'll be nice when we finally get rid of these wrapper classes

Yeah, right now, we have to keep our UI usable.

slluis · 2018-02-22T13:19:41Z

@Therzok can you fix the conflicts?

…kspace provider Also fix refactorings taking into account options UI.

…zation

…agnostics fixes

Therzok · 2018-02-22T13:26:37Z

Done!

Therzok force-pushed the codefix branch 2 times, most recently from ca059f9 to 0841be7 Compare January 12, 2018 14:31

Therzok requested review from KirillOsenkov, mhutch and mkrueger January 13, 2018 09:58

mkrueger approved these changes Jan 15, 2018

View reviewed changes

Therzok mentioned this pull request Jan 15, 2018

Text change failure in TextDocument tests #3652

Closed

Therzok force-pushed the codefix branch from a22803c to 5e0816c Compare January 15, 2018 15:41

Therzok force-pushed the codefix branch 4 times, most recently from 5d9b236 to 51b4483 Compare January 18, 2018 14:59

Therzok added the qa-pending label Jan 18, 2018

Therzok force-pushed the codefix branch from 4489de1 to f449a46 Compare January 19, 2018 07:57

garuma mentioned this pull request Feb 14, 2018

Red squiggles have a tendency to stick around #3858

Closed

Therzok force-pushed the codefix branch 2 times, most recently from 0b5a747 to 30e1a90 Compare February 15, 2018 11:45

KirillOsenkov approved these changes Feb 15, 2018

View reviewed changes

mhutch approved these changes Feb 16, 2018

View reviewed changes

Therzok added 2 commits February 22, 2018 15:25

[Refactoring] Switch to Roslyn CodeFixService

a6e88c9

Flush some fixes and cleanups

e1f4ae9

Therzok added 7 commits February 22, 2018 15:25

[Ide] More dead code removal

0611d69

[Refactoring] Delete dead code, migrate options to the diagnostic wor…

621cf13

…kspace provider Also fix refactorings taking into account options UI.

Fix a NRE

fa3b804

[Refactoring] Fix code issues still appearing for suppressed diagnostics

435589b

This file was not committed

861e8d9

[Refactoring] Some updates to prevent some possible NREs and uniformi…

c63be61

…zation

[Ide] Re-enable new error backend for C# and add test for compiler di…

2457c90

…agnostics fixes

Therzok force-pushed the codefix branch from e2c4e9c to 2457c90 Compare February 22, 2018 13:26

Therzok merged commit 1ba94df into editorFeatures Feb 23, 2018

Therzok deleted the codefix branch February 23, 2018 14:37

sgmunn added qa-pending and removed qa-pending labels Mar 6, 2018

xamarin-release-manager mentioned this pull request Sep 29, 2018

NullRef in TextEditor.cs:96 #5999

Closed

Conversation

Therzok commented Jan 12, 2018

Uh oh!

mkrueger left a comment

Choose a reason for hiding this comment

Uh oh!

Therzok commented Jan 15, 2018

Uh oh!

Therzok commented Jan 15, 2018

Uh oh!

Therzok commented Jan 16, 2018

Uh oh!

Therzok commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Therzok commented Jan 18, 2018

Uh oh!

Therzok commented Feb 15, 2018

Uh oh!

Therzok commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhutch commented Feb 16, 2018

Uh oh!

mhutch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slluis commented Feb 22, 2018

Uh oh!

Therzok commented Feb 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Therzok commented Jan 16, 2018 •

edited

Loading

Therzok commented Feb 15, 2018 •

edited

Loading