Various loose ends in preparation for merging refout feature #19326

jcouv · 2017-05-07T00:40:40Z

A few things:

xml docs are not filtered when using /refonly (updated doc and tests to reflect)
NoPia is disallowed with either /refonly or /refout
compacting error codes for C# 7.1 (and fixing a bug in test for warnings)
emit ref assemblies with determinism, even if compilation isn't

@dotnet/roslyn-compiler for review. Thanks

jcouv · 2017-05-07T21:43:55Z

src/Compilers/CSharp/Test/Emit/Emit/CompilationEmitTests.cs


+        [Theory]
+        [InlineData("")]
+        [InlineData(@"[assembly: System.Reflection.AssemblyVersion(""1"")]")]


It seems that the /deterministic flag affects two things: (1) whether wildcards are allowed in AssemblyVersion attribute, and (2) how PEs are emitted.
Would you know what the rationale is for (1)? It doesn't seem "non-deterministic".

See the docs:

You can specify all the values or you can accept the default build number, revision number, or both by using an asterisk (*). [...] The default build number increments daily. The default revision number is the number of seconds since midnight local time (without taking into account time zone adjustments for daylight saving time), divided by 2.

That is nondeterministic.

Thanks for the clarification.
I see that VersionHelper.GenerateVersionFromPatternAndCurrentTime replaces the wildcard (maxValue) with the timestamp of the build.
That means if you use a wildcard, the ref assembly will not be deterministic. I wonder if I should switch the compilation to be deterministic if either /refonly or /refout is used.

jcouv · 2017-05-07T21:56:36Z

src/Compilers/CSharp/Portable/Emitter/Model/NamedTypeSymbolAdapter.cs


            foreach (var member in this.GetMembers())
            {
-                // PROTOTYPE(refout) Do something here?


📝 Explicit interface implementations should be included. My understanding is that they are virtual, and we already concluded that all virtual methods should be included in ref assemblies. I'll add a test to lock that in.

gafter

I have just the one question. The rest looks good.

gafter · 2017-05-08T04:10:18Z

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

        <member name=""M:C.Main"">
            <summary>Main method</summary>
        </member>
+        <member name=""F:C.field"">


This is unexpected. We don't eliminate private fields of a class?

Such fields are stripped from the ref assembly, but not from the doc output. I'll take a stab tomorrow to strip it out.

cston · 2017-05-08T15:27:23Z

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs

                AddDiagnostic(diagnostics, diagnosticOptions, ErrorCode.ERR_NoRefOutWhenRefOnly);
            }

+            if (metadataReferences.Any(r => r.Properties.EmbedInteropTypes) && (refOnly || outputRefFilePath != null))


Perhaps check second && operand first since that might be cheaper.

cston · 2017-05-08T15:47:48Z

src/Compilers/VisualBasic/Portable/CommandLine/VisualBasicCommandLineParser.vb

                AddDiagnostic(diagnostics, ERRID.ERR_NoRefOutWhenRefOnly)
            End If

+            If metadataReferences.Any(Function(r) r.Properties.EmbedInteropTypes) AndAlso (refOnly OrElse outputRefFileName IsNot Nothing) Then


Consider checking second AndAlso operand first.

cston · 2017-05-08T15:48:41Z

src/Compilers/VisualBasic/Portable/VBResources.resx

    <value>Do not use refout when using refonly.</value>
  </data>
+  <data name="ERR_NoEmbeddedTypeWhenRefOutOrRefOnly" xml:space="preserve">
+    <value>Cannot compile embed types when using /refout or /refonly.</value>


Cannot embed types ...

cston · 2017-05-08T15:49:44Z

LGTM

… assembly. Hook assembly redirect.

jcouv · 2017-05-09T03:02:31Z

@gafter @cston Thanks for the feedback.

I added a fix for assembly redirect. This is a problem with System.IO.FileSystem, which I also encountered in the corefx repo. More details in this issue https://github.com/dotnet/corefx/issues/16322#issuecomment-286179535.
For the last couple of days, I had used a workaround (adding assembly redirect in msbuild.exe.config) so that CopyRefAssembly could load System.IO.FileSystem. But from discussion with @agocke, we already use a more robust solution for Csc and Vbc task. I'm applying the same trick in CopyRefAssembly. Verified it works locally and a VM.

I'll merge as soon as green, so that I can produce a new VS validation build.

jcouv · 2017-05-09T03:33:32Z

Build correctness leg failed with:

20:08:59 Running BuildBoss
20:09:00 Processing Solution Roslyn.sln ... passed
20:09:00 Processing Solution Compilers.sln ... passed
20:09:00 Processing Solution src\Samples\Samples.sln ... passed
20:09:00 Processing Solution CrossPlatform.sln ... passed
20:09:00 Processing Targets build\Targets ... passed
20:09:14 Command failed to execute: D:\j\workspace\windows_build---59ebb5ea\Binaries\Debug\Exes\BuildBoss\BuildBoss.exe Roslyn.sln Compilers.sln src\Samples\Samples.sln CrossPlatform.sln build\Targets D:\j\workspace\windows_build---59ebb5ea\Binaries\Debug\build.xml
20:09:14 System.Management.Automation.RuntimeException: Command failed to execute: D:\j\workspace\windows_build---59ebb5ea\Binaries\Debug\Exes\BuildBoss\BuildBoss.exe Roslyn.sln Compilers.sln src\Samples\Samples.sln CrossPlatform.sln build\Targets D:\j\workspace\windows_build---59ebb5ea\Binaries\Debug\build.xml
20:09:14 Command failed to execute:  & ".\build\scripts\test-build-correctness.ps1" -config $buildConfiguration 
20:09:14 System.Management.Automation.RuntimeException: Command failed to execute:  & ".\build\scripts\test-build-correctness.ps1" -config $buildConfiguration 
20:09:14 Build step 'Execute Windows batch command' marked build as failure

I'll re-run.

jcouv · 2017-05-09T03:33:42Z

@dotnet-bot test windows_build_correctness_prtest please

Improve xmldoc tests. Disallow NoPia.

99f8b03

jcouv added the Area-Compilers label May 7, 2017

jcouv added this to the 15.3 milestone May 7, 2017

jcouv self-assigned this May 7, 2017

dnfclas added the cla-already-signed label May 7, 2017

Ref assemblies should always be deterministic

f73453b

jcouv force-pushed the refout-prototype branch from 4416d9a to 7205ec6 Compare May 7, 2017 21:31

Julien Couvreur added 2 commits May 7, 2017 14:39

Fixing CopyRefAssembly message output

ed29ec4

Compact error codes for C# 7.1

6c5b341

jcouv force-pushed the refout-prototype branch from 7205ec6 to 6c5b341 Compare May 7, 2017 21:39

jcouv commented May 7, 2017

View reviewed changes

Tweaks

d881d9e

jcouv assigned gafter May 7, 2017

jcouv commented May 7, 2017

View reviewed changes

gafter reviewed May 8, 2017

View reviewed changes

cston reviewed May 8, 2017

View reviewed changes

Addressing feedback from Chuck. Handle scenario where input isn't ref…

1993456

… assembly. Hook assembly redirect.

jcouv force-pushed the refout-prototype branch from 19ca82f to 1993456 Compare May 9, 2017 03:01

jcouv merged commit 8af036a into dotnet:features/refout May 9, 2017

jcouv deleted the refout-prototype branch May 9, 2017 04:33

Various loose ends in preparation for merging refout feature #19326

Various loose ends in preparation for merging refout feature #19326

Uh oh!

Conversation

jcouv commented May 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

gafter 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!

cston commented May 8, 2017

Uh oh!

jcouv commented May 9, 2017

Uh oh!

jcouv commented May 9, 2017

Uh oh!

jcouv commented May 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jcouv commented May 7, 2017 •

edited

Loading