dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.13k stars 4.04k forks source link

MSBuildWorkspace does not cleanly load VB project on Linux or MacOS #72014

Open JoeRobich opened 10 months ago

JoeRobich commented 10 months ago

Version Used: 4.10 Preview 1

Reported in https://github.com/dotnet/roslyn/pull/69225

Steps to Reproduce:

  1. dotnet new console --language VB
  2. use MSBuildWorkspace.OpenProjectAsync to load the newly created VB project

Expected Behavior: Project loads without any workspace diagnostics and compilation reports no compiler diagnostics.

Actual Behavior: Compilation reports "BC30002": Type 'Global.Microsoft.VisualBasic.ApplicationServices.ApplicationBase' is not defined.

paul1956 commented 9 months ago

@JoeRobich I believe that is part of the WinForms VB Runtime not .Net Core, there have been some requests to move it and other parts of the VB Runtime down to Core since it would enable easier access from C# and allow VB code on Linux and Mac, but I have not seen any movement. There is very little in ApplicationBase (120 lines) but ApplicationBase.Info uses reflection on Windows to access Assembly and if you need that more work would be required.

arunchndr commented 1 week ago

Joey - please move this to the runtime repo when ready.

JoeRobich commented 6 days ago

After doing a little more investigation, this isn't an issue for the Runtime or WinForms teams.

  1. The reported diagnostics are located in the "MyTemplateLocation", meaning the compiler provided template.
  2. The compiler doesn't report any diagnostics when performing a command line build.
  3. The binarylog of the command line build shows the _MyType should be defined as "Empty" Image
  4. The MyTemplate should not reference 'Global.Microsoft.VisualBasic.ApplicationServices.ApplicationBase' or any other type when _MyType is "Empty"
  5. The MSBuildWorkspace loaded project's ParseOptions show only 2 PreprocessorSymbol and neither are _MyType
  6. When _MyType is "" the template treats it as a Windows type which explains why it is referencing these classes that aren't available.

So why are we not getting the proper _MyType?

  1. The projectFileInfo.CommandLineArgs loaded from the ProjectFile in MSBuildProjectLoader.Worker shows many defined symbols but some odd character escaping.
  2. Checking the json read in RpcClient, the define looks like "/define:\"CONFIG=/\"Debug/\",DEBUG=-1,TRACE=-1,PLATFORM=/\"AnyCPU/\",NET=-1,NET9_0=-1,NETCOREAPP=-1,_MyType=/\"Empty/\",NET5_0_
  3. My type is defined but oddly escaped _MyType=/\"Empty/\"
  4. The CommandLineArgs come from reading the VbcCommandLineArgs Item from the design time build.
  5. Checking the VbcCommandLineArgs Item from a design-time build binlog, _MyList is report /"Empty/" Image
  6. Comparing this to the Vbc task's CommandLineArguments and I see the quotes properly escaped. Image
  7. This is happening on non-Windows platforms because MSBuild is "fixing up" the ItemSpec (replacing the Windows default path separator '\' with '/') when creating the VbcCommandLineArgs TaskItem.
  8. The JsonSerializer is then turns /"Empty/" into /\"Empty/\" when escaping the double quotes.
  9. Going back to the MSBuildProjectLoader.Worker and inspecting the parsed commandLineArgs.Errors there is indeed an error that is not surfaced.
    error BC31030: Conditional compilation constant 'CONFIG= ^^ ^^ /"Debug/"' is not valid: Syntax error in conditional compilation expression.

Options:

  1. We could stop using the *CommandLineArgs Items as we already maintain *CommandLineArgumentReader classes which read build Properties (importantly not Items) to gather the same information.
  2. We could treat /" in the CommandLineArgs ItemSpec as an escaped double quote. However, there might be false positives.
  3. MSBuild could change the ItemSpec behavior. Likely the riskiest option at this point.

cc: @jasonmalinowski & @rainersigwald for thoughts

jasonmalinowski commented 2 days ago

Is that TaskItem constructor being called here?

https://github.com/dotnet/roslyn/blob/fac15a8b5c8bfb92aa94fa337e8bfe804526495d/src/Compilers/Core/MSBuildTask/ManagedToolTask.cs#L162

Because if so, it looks like there's another constructor that doesn't do escaping:

https://github.com/dotnet/msbuild/blob/4c6a5a963ceb38f77af4e57d28669872b616a8dc/src/Utilities/TaskItem.cs#L88-L96

Can we call that one instead?

JoeRobich commented 1 day ago

Can we call that one instead?

Unfortunately it does call the other constructor which does the escaping. https://github.com/dotnet/msbuild/blob/4c6a5a963ceb38f77af4e57d28669872b616a8dc/src/Utilities/TaskItem.cs#L100

We could create out own ITaskItem implementation to wrap the args and pass it to this constructor. https://github.com/dotnet/msbuild/blob/4c6a5a963ceb38f77af4e57d28669872b616a8dc/src/Utilities/TaskItem.cs#L120-L123

Also, thank you for linking me to where this TaskItem was being created on our end.

jasonmalinowski commented 1 day ago

@rainersigwald Any thoughts here? Invoking that copy constructor seems like a possible workaround.

rainersigwald commented 10 hours ago

. . . how on Earth is this the first time that unconditional correction has come up? We should definitely have an API to avoid it. (edit: https://github.com/dotnet/msbuild/issues/11083)

I think you're right that the best workaround is a custom ITaskItem--I don't think you'll need to create a TaskItem from it though, you should be able to return your type IIRC.