fix(roslyn): drop Workspaces-layer formatting so USE_ROSLYN compiles with the installer's DLLs - #1404
Open
comedianhhh wants to merge 1 commit into
Open
comedianhhh wants to merge 1 commit into
comedianhhh wants to merge 1 commit into
Conversation
…with installer DLLs ManageScript's optional post-validation formatting used AdhocWorkspace and Microsoft.CodeAnalysis.Formatting.Formatter, which live in the Roslyn Workspaces assemblies. RoslynInstaller only ships the compiler layer (Microsoft.CodeAnalysis + CSharp) and the asmdef only references those, so enabling USE_ROSLYN via the recommended installer path fails with CS0234. Remove the formatting block (it was best-effort inside try/catch and is not part of the documented Roslyn validation feature) and add a regression test that keeps Workspaces-layer APIs out of MCPForUnity.Editor. Fixes CoplayDev#1391
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change removes Roslyn formatting from script edits while retaining syntax validation. It adds a test that rejects Roslyn Workspaces references in editor sources and extracts asmdef path lookup into a helper. ChangesRoslyn Workspaces Removal
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change removes the unsupported Workspaces formatting dependency and adds a guard against reintroducing those APIs. No actionable merge risk remains in the reviewed scope. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Enabling
USE_ROSLYNafter the one-click installer fails withManageScript.cs(17,30): error CS0234 ... 'Formatting'. The optional post-validation formatting usesAdhocWorkspace+Formatter.Format, which live in the Roslyn Workspaces assemblies;RoslynInstallerand the asmdef only ship/reference the compiler layer. Removing the formatting block (best-effort, insidetry/catch, not part of the documented validation feature) fixes the build without pulling Workspaces and its transitive deps into the installer.Type of Change
Changes Made
ManageScript.cs: removeusing Microsoft.CodeAnalysis.Formattingand theAdhocWorkspace/Formatter.Formatblock under#if USE_ROSLYN.RoslynAsmdefReferenceTests.cs: addEditorSources_DoNotUseRoslynWorkspacesLayer(fails ifMCPForUnity/EditorreferencesMicrosoft.CodeAnalysis.Formatting,Microsoft.CodeAnalysis.Workspaces, orAdhocWorkspace); share the asmdef path lookup.Compatibility / Package Source
file:../../../MCPForUnityTesting/Screenshots/Recordings
Assets/Plugins/Roslyn/+-define:USE_ROSLYN→ the exact CS0234 from the issue.USE_ROSLYN: EditMode 1244 total / 1162 passed / 80 skipped / 2 failed. The 2 failures (ExecuteCodeTests.Execute_CodedomBackend_*, "YamlDotNet.dll does not contain valid metadata") fail identically with the define off, so they are environmental.Documentation Updates
Related Issues
Fixes #1391
Additional Notes
Adding
Microsoft.CodeAnalysis.CSharp.Workspaces.dllto the asmdef alone would not be enough —AdhocWorkspaceandFormatterare inMicrosoft.CodeAnalysis.Workspaces.dll, which is not shipped either. If auto-formatting should stay,root.NormalizeWhitespace()is the compiler-layer alternative, but it reformats much more aggressively, so I did not swap it in silently.Summary by CodeRabbit
Bug Fixes
Tests