Remove more private reflection from compiler #32257

jaredpar · 2019-01-08T23:24:39Z

This change continues the removal of private reflection code from the compiler code base and instead relying on mulit-targeting to have detect the runtime. This also necessarily cleans up a number of our #if checks in the code base.

Follow up to #31763

Use multi-target instead of reflection

This removes the remaining private reflection code from the compiler and replaces it with multi-targeting approaches.

jaredpar · 2019-01-08T23:24:54Z

CC @dotnet/roslyn-compiler for review

src/Test/Utilities/Portable/Compilation/RuntimeUtilities.cs

jcouv · 2019-01-11T19:04:27Z

src/Compilers/Shared/BuildServerConnection.cs

-                var expectedCompilerPath = Path.Combine(clientDir, ServerNameCoreClr);
-                expectedPath = Environment.GetEnvironmentVariable("DOTNET_HOST_PATH") ?? "dotnet";
-                processArguments = $@"""{expectedCompilerPath}"" ""-pipename:{pipeName}""";
+            var serverPathWithoutExetnsion = Path.Combine(clientDir, "VBCSCompiler");


serverPathWithoutExetnsion [](start = 16, length = 26)

typo: Extension #Resolved

jcouv · 2019-01-11T19:09:25Z

src/Test/Utilities/Portable/TargetFrameworkUtil.cs

@@ -55,7 +56,7 @@ public enum TargetFramework

    public static class TargetFrameworkUtil
    {
-        public static MetadataReference StandardCSharpReference => CoreClrShim.IsRunningOnCoreClr ? NetStandard20.MicrosoftCSharpRef : TestBase.CSharpDesktopRef;


CoreClrShim.IsRunningOnCoreClr [](start = 67, length = 30)

It looks like you're replacing all uses of CoreClrShim.IsRunningOnCoreClr. Can that API be removed then? #WontFix

No. Scripting still uses it and pulling it out from there is a lot of work. It's on my list but wanted it to be a separate change as it's likely a bit of refactoring. #WontFix

jcouv · 2019-01-11T19:22:53Z

src/Test/Utilities/Portable/Assert/UseCultureAttribute.cs

            _culture = new Lazy<CultureInfo>(() => new CultureInfo(culture, useUserOverride: false));
            _uiCulture = new Lazy<CultureInfo>(() => new CultureInfo(uiCulture, useUserOverride: false));
-#elif NETCOREAPP2_1
-            _culture = new Lazy<CultureInfo>(() => new CultureInfo(culture));


)); [](start = 74, length = 3)

We used to have a distinction of useUserOverride: being true or false. Did something change that allows us to no longer care? #ByDesign

The original code here preferred the useUserOverride: false approach. But when we went to netcoreapp1.1 originally that overload wasn't present and we were forced to pick a different one. Now with netcoreapp2.1 the overload is back and we can go with our original preferred approach. #ByDesign

jcouv · 2019-01-11T19:25:30Z

src/Compilers/Shared/RuntimeHostInfo.cs

+        /// This gets information about invoking a tool on the current runtime. This will attempt to 
+        /// execute a tool as an EXE when on desktop and using dotnet when on CoreClr.
+        /// </summary>
+        internal static (string ProcessFilePath, string CommandLineArguments, string ToolFilePath) GetProcessInfo(string toolFilePathWithoutExtension, string commandLineArguments)


ProcessFilePath [](start = 32, length = 15)

nit: tuple names should be lower-cased (parallel with parameter names) #Resolved

jcouv · 2019-01-11T19:26:33Z

src/Compilers/Shared/RuntimeHostInfo.cs

+            Debug.Assert(!toolFilePathWithoutExtension.EndsWith(".dll") && !toolFilePathWithoutExtension.EndsWith(".exe"));
+
+            var toolFilePath = $"{toolFilePathWithoutExtension}.{ToolExtension}";
+            if (IsDotnetHost(out string pathToDotNet))


nit: Dotnet vs. DotNet. This line has both conventions #Resolved

I've confirmed that Dotnet is preferred. Will use that. #Resolved

jcouv · 2019-01-11T19:29:40Z

src/Compilers/Shared/RuntimeHostInfo.cs

+        /// Get the path to the dotnet executable. In the case the host did not provide this information
+        /// in the environment this will return simply "dotnet".
+        /// </summary>
+        internal static string GetDotnetPathOrDefault()


internal [](start = 8, length = 8)

private? #Resolved

jcouv · 2019-01-11T19:29:50Z

src/Compilers/Shared/RuntimeHostInfo.cs

+        /// Get the path to the dotnet executable. This will throw in the case it is not properly setup 
+        /// by the environment.
+        /// </summary>
+        internal static string GetDotnetPath()


internal [](start = 8, length = 8)

private?
Same comment for IsDotnetHost and DotnetHostPathEnvironmentName above

jcouv

LGTM Thanks (iteration 6)

jaredpar · 2019-01-15T01:06:01Z

@roslyn-compiler could I get one more review here?

After more searching it is clear that DotNet is preferred in API signatures over Dotnet. Easy to see when you look at the reference source https://referencesource.microsoft.com/#q=dotnet

RikkiGibson · 2019-01-15T21:37:39Z

src/Compilers/CSharp/csc/csc.csproj

    <Compile Include="..\..\Shared\NamedPipeUtil.cs" />
-    <Compile Include="..\..\Shared\BuildClient.cs">
-      <Link>BuildClient.cs</Link>


This is documented in https://docs.microsoft.com/en-us/visualstudio/msbuild/common-msbuild-project-items?view=vs-2017 as follows:
Link | Optional string. The notational path to be displayed when the file is physically located outside the influence of the project file.

Does this mean this only affects the build logs, not the behavior?

I'm unsure if it affects the build logs or not but it does affect the UI in Visual Studio. You can use this field to change how the file is displayed, and in what folder, in the VS solution explore.

The name for the named pipe used between MSBuild and VBCSCompiler is generated from a combination of values including the current user name, specifically `%USERNAME%`. It is possible for this value to have spaces in it and hence the argument must be quoted when passing it to the command line of VBCSCompiler instances. Regression initially introduced: dotnet#32257

Remove more private reflection from compiler

jaredpar added 5 commits January 8, 2019 15:18

Fix multi-target

ebd4e3d

Use multi-target instead of reflection

Remove private reflection in the compiler

6620be0

This removes the remaining private reflection code from the compiler and replaces it with multi-targeting approaches.

Clean up project files

f0b8d80

More cleanup

b62e661

Respond to PR feedback

1d8334b

jaredpar added the Area-Compilers label Jan 8, 2019

jaredpar requested review from a team as code owners January 8, 2019 23:24

Fix extension handling

ea900cc

RikkiGibson reviewed Jan 9, 2019

View reviewed changes

src/Test/Utilities/Portable/Compilation/RuntimeUtilities.cs Show resolved Hide resolved

jcouv added this to the 16.0.P3 milestone Jan 11, 2019

jcouv reviewed Jan 11, 2019

View reviewed changes

jcouv approved these changes Jan 11, 2019

View reviewed changes

jcouv self-assigned this Jan 11, 2019

Respond to PR feedback

bed1068

Use DotNet instead of Dotnet

ff96f4f

After more searching it is clear that DotNet is preferred in API signatures over Dotnet. Easy to see when you look at the reference source https://referencesource.microsoft.com/#q=dotnet

RikkiGibson reviewed Jan 15, 2019

View reviewed changes

RikkiGibson approved these changes Jan 15, 2019

View reviewed changes

jaredpar merged commit 9e012fe into dotnet:master Jan 16, 2019

jaredpar deleted the fix-reflection branch January 16, 2019 19:31

jaredpar mentioned this pull request Feb 18, 2019

Ensure pipename argument quoted #33474

Merged

jaredpar mentioned this pull request Feb 18, 2019

Ensure pipename argument quoted #33475

Merged

xoofx pushed a commit to stark-lang/stark-roslyn that referenced this pull request Apr 16, 2019

Merge pull request dotnet#32257 from jaredpar/fix-reflection

8fe1a60

Remove more private reflection from compiler

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove more private reflection from compiler #32257

Remove more private reflection from compiler #32257

jaredpar commented Jan 8, 2019

jaredpar commented Jan 8, 2019

jcouv Jan 11, 2019 •

edited by jaredpar

Loading

jcouv Jan 11, 2019 •

edited by jaredpar

Loading

jaredpar Jan 11, 2019 •

edited

Loading

jcouv Jan 11, 2019 •

edited by jaredpar

Loading

jaredpar Jan 11, 2019 •

edited

Loading

jcouv Jan 11, 2019 •

edited by jaredpar

Loading

jcouv Jan 11, 2019 •

edited by jaredpar

Loading

jaredpar Jan 11, 2019 •

edited

Loading

jcouv Jan 11, 2019 •

edited by jaredpar

Loading

jcouv Jan 11, 2019 •

edited

Loading

jcouv left a comment

jaredpar commented Jan 15, 2019

RikkiGibson Jan 15, 2019

jaredpar Jan 16, 2019

Remove more private reflection from compiler #32257

Remove more private reflection from compiler #32257

Conversation

jaredpar commented Jan 8, 2019

jaredpar commented Jan 8, 2019

jcouv Jan 11, 2019 • edited by jaredpar Loading

Choose a reason for hiding this comment

jcouv Jan 11, 2019 • edited by jaredpar Loading

Choose a reason for hiding this comment

jaredpar Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv Jan 11, 2019 • edited by jaredpar Loading

Choose a reason for hiding this comment

jaredpar Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv Jan 11, 2019 • edited by jaredpar Loading

Choose a reason for hiding this comment

jcouv Jan 11, 2019 • edited by jaredpar Loading

Choose a reason for hiding this comment

jaredpar Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv Jan 11, 2019 • edited by jaredpar Loading

Choose a reason for hiding this comment

jcouv Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jaredpar commented Jan 15, 2019

RikkiGibson Jan 15, 2019

Choose a reason for hiding this comment

jaredpar Jan 16, 2019

Choose a reason for hiding this comment

jcouv Jan 11, 2019 •

edited by jaredpar

Loading

jcouv Jan 11, 2019 •

edited by jaredpar

Loading

jaredpar Jan 11, 2019 •

edited

Loading

jcouv Jan 11, 2019 •

edited by jaredpar

Loading

jaredpar Jan 11, 2019 •

edited

Loading

jcouv Jan 11, 2019 •

edited by jaredpar

Loading

jcouv Jan 11, 2019 •

edited by jaredpar

Loading

jaredpar Jan 11, 2019 •

edited

Loading

jcouv Jan 11, 2019 •

edited by jaredpar

Loading

jcouv Jan 11, 2019 •

edited

Loading