Compilation: use System.Object from target corlib #8507

abock · 2016-02-09T21:43:28Z

When creating a script compilation without an explicit return type, System.Object was being resolved via reflection from the host.

This resulted in an implicit dependency of a script compilation on the host corlib, even if a different corlib was specified as a reference for the compilation (e.g. Xamarin.iOS).

Fix this by using System.Object as defined in the corlib resolved for the compilation.

This fixes #8506.

davkean · 2016-02-10T02:35:15Z

@dotnet-bot test eta please
@dotnet-bot test vsi please

davkean · 2016-02-10T02:35:55Z

tag @dotnet/roslyn-compiler @dotnet/roslyn-interactive

abock · 2016-02-10T06:24:49Z

@tmat updated with a test and explicitly redirecting typeof(object).

I had to provide another test corlib since none of the existing ones satisfied a shape like the Xamarin mobile profile corlibs:

assembly version cannot be 4.0.0.0 (Xamarin mobile corlibs are 2.0.5.0)
must provide Task.GetAwaiter(), Task<T> {}, and TaskAwaiter.GetResult() stubs to support the script host

I will update this PR again with the VB side later.

Without the fix, the test fails as it did for me realistically as when cross compiling for Xamarin.iOS:

Microsoft.CodeAnalysis.CSharp.UnitTests.CompilationAPITests.CrossCorlibSystemObjectReturnType_Script [FAIL]

Expected:
Actual:
// error CS0400: The type or namespace name 'System.Object, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089' could not be found in the global namespace (are you missing an assembly reference?)
Diagnostic(ErrorCode.ERR_GlobalSingleTypeNameNotFound).WithArguments("System.Object, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089").WithLocation(1, 1)
Diff:
++>     Diagnostic(ErrorCode.ERR_GlobalSingleTypeNameNotFound).WithArguments("System.Object, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089").WithLocation(1, 1)
Expected: True
Actual:   False
Stack Trace:
<filename unknown>(0,0): at Microsoft.CodeAnalysis.DiagnosticExtensions.Verify (IEnumerable`1 actual, Microsoft.CodeAnalysis.Test.Utilities.DiagnosticDescription[] expected, Boolean errorCodeOnly)
<filename unknown>(0,0): at Microsoft.CodeAnalysis.DiagnosticExtensions.Verify (IEnumerable`1 actual, Microsoft.CodeAnalysis.Test.Utilities.DiagnosticDescription[] expected)
<filename unknown>(0,0): at Microsoft.CodeAnalysis.DiagnosticExtensions.Verify (ImmutableArray`1 actual, Microsoft.CodeAnalysis.Test.Utilities.DiagnosticDescription[] expected)
<filename unknown>(0,0): at Microsoft.CodeAnalysis.DiagnosticExtensions.VerifyDiagnostics[TCompilation] (Microsoft.CodeAnalysis.TCompilation c, Microsoft.CodeAnalysis.Test.Utilities.DiagnosticDescription[] expected)
<filename unknown>(0,0): at Microsoft.CodeAnalysis.CSharp.UnitTests.CompilationAPITests.CrossCorlibSystemObjectReturnType_Script ()
at (wrapper managed-to-native) System.Reflection.MonoMethod:InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&)
<filename unknown>(0,0): at System.Reflection.MonoMethod.Invoke (System.Object obj, BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture)

davkean · 2016-02-10T07:01:10Z

@dotnet-bot test eta please
@dotnet-bot test vsi please

abock · 2016-02-10T07:08:59Z

@tmat updated with VB patch and test, and while they both compile just fine I'm shooting in the dark:

this is the first VB I've ever written
the VB tests do not seem to run at all (even if I fiddle with build/scripts/tests.sh) on Mac. Seems every test fails

I do not have a Windows machine readily available with a build environment 😄

abock · 2016-02-10T07:10:39Z

@tmat also FYI, if you comment on the PR diff itself (Files changed tab) instead of individual commits (Commits tab) then comments will stick around in the PR when pushing a rebased patch.

abock · 2016-02-10T23:03:43Z

rebased to fix merge conflict with master

AlekseyTs · 2016-02-10T23:38:27Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedInteractiveInitializerMethod.cs

+            // corlib's System.Object. This allows cross compiling scripts to build on one
+            // corlib and run on another.
+            // cf. https://github.com/dotnet/roslyn/issues/8506
+            resultType = submissionReturnType == typeof(object)


It feels to me that compilation.GetTypeByReflectionType should locate proper symbol without a need to special case System.Object. Otherwise, why other special types not special cased (System.Int32, for example)? Perhaps submissionReturnType is set to the wrong Type instance, perhaps that is what should be fixed?

This is the right approach. We only handle system.object here because typeof(object) is filled in by default if the host doesn't specify the type. If the host specifies a rutnime type explicitly it has to match the corresponding type in referenced metadata exactly.

It sounds like we would want the code to fail if host specified typeof(object) explicitly, but with this change the code would succeed. I would suggest to change the line

var submissionReturnType = compilation.SubmissionReturnType ?? typeof(object);

to not overwrite null with typeof(object) and base the current logic on whether submissionReturnType is null instead.

@AlekseyTs brings up a good point. We can change ScriptCompilationInfo like so:

public Type ReturnType => ReturnTypeOpt ?? typeof(object); internal Type ReturnTypeOpt { get; } internal ScriptCompilationInfo(Type returnType, Type globalsType) { ReturnTypeOpt = returnType; GlobalsType = globalsType; }

and then in CalculateReturnType we can replace the first line with

var submissionReturnTypeOpt = compilation.ScriptCompilationInfo.ReturnTypeOpt;

and avoid equality check with typeof(object).

Note that ScriptCompilationInfo should always be non-null here, since the method is only used for compiling submissions.

tmat · 2016-02-11T00:20:25Z

👍

davkean · 2016-02-11T15:37:32Z

Both unit64/unit32 timed out here, with a deadlock in Roslyn.Compilers.CSharp.Symbol.UnitTests.dll. Unfortunately, we don't have dumps as our infrastructure isn't setup to catch them.

These look like real failures. Do you have a Windows machine to repro this locally?

tmat · 2016-02-11T17:17:21Z

The failure is not likely related to this change

tmat · 2016-02-12T20:30:26Z

@dotnet-bot retest prtest/win/dbg/unit32 please
// Previous failure: http://dotnet-ci.cloudapp.net/job/roslyn_prtest_win_dbg_unit32/3644/
// Retest reason:

tmat · 2016-02-12T22:55:49Z

test this please

abock · 2016-02-16T19:14:11Z

Just an FYI, I haven't been able to get back to this until now. I'll come back to this PR to encapsulate the rest of the feedback sometime this week.

I have set up a Windows build environment and was able to test the patch as-is just fine. No local failures. Subsequently, I opened a new PR to fix the test runner invocation:

#8744

tmat · 2016-05-12T22:14:51Z

FYI: #11279

abock · 2016-05-12T23:12:57Z

@tmat cool, thanks, I'll fix that. I was not able to see the failure locally (Mac) and on CI there was nothing obvious - just timing out.

tmat · 2016-05-12T23:32:44Z

We currently run only a small subset of tests on Mac :(

abock · 2016-05-13T00:00:34Z

Okay, now the failures are legitimate and sensible, but look like the result of taking a previous comment's change suggestion into account. I'll get back to a Windows machine tomorrow to take a look.

jasonmalinowski · 2016-05-13T00:12:11Z

@tmat: failed assertions should be throwing exceptions instead of popping dialogs. This thread is long and windy and not quite sure what I can look at to see what you're seeing.

tmat · 2016-05-13T00:25:29Z

@jasonmalinowski Here: http://dotnet-ci.cloudapp.net/job/roslyn_prtest_win_dbg_unit32/7049/

abock · 2016-06-01T01:15:26Z

@tmat @davkean @AlekseyTs updated again, VB and C# tests passing locally

This combines the async features of minasync with mincorlib to produce a minimum unversioned corlib with async stubs.

When creating a script compilation without an explicit return type, System.Object was being resolved via reflection from the host. This resulted in an implicit dependency of a script compilation on the host corlib, even if a different corlib was specified as a reference for the compilation (e.g. Xamarin.iOS). Fix this by using System.Object as defined in the corlib resovled for the compilation.

davkean · 2016-06-01T04:22:58Z

@dotnet-bot test eta please
@dotnet-bot test vsi please

davkean · 2016-06-01T04:23:39Z

@tmat Can you look at the recent commits?

tmat · 2016-06-01T06:04:32Z

👍

tmat · 2016-06-01T06:05:21Z

test windows_vsi_p2_open_prtest please

AlekseyTs · 2016-06-01T18:49:05Z

LGTM.
@ManishJayaswal If we want this change to go to Update 3, we need to get Ask mode approval and retarget this change to stabilization.

MattGertz · 2016-06-02T01:37:41Z

Yep, let's do it.

tmat · 2016-06-02T01:42:51Z

Merging to master and porting to stabilization.

* Tests: fix minasync Task<T> to derive from Task * Tests: provide MinAsyncCorlibRef This combines the async features of minasync with mincorlib to produce a minimum unversioned corlib with async stubs. * Compilation: use System.Object from target corlib When creating a script compilation without an explicit return type, System.Object was being resolved via reflection from the host. This resulted in an implicit dependency of a script compilation on the host corlib, even if a different corlib was specified as a reference for the compilation (e.g. Xamarin.iOS). Fix this by using System.Object as defined in the corlib resovled for the compilation.

davkean · 2016-06-02T04:41:43Z

@abock Know that this was a long effort, but thanks a lot for the contribution! Hope this means you can move to public bits, instead of privately built bits.

abock · 2016-06-02T17:15:19Z

@davkean thanks! We will definitely now be able to take builds directly from upstream with this patch now landed.

Thanks @tmat as well!

dnfclas added the cla-signed label Feb 9, 2016

davkean added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Feb 10, 2016

ManishJayaswal added this to the 1.3 milestone Feb 10, 2016

ManishJayaswal assigned tmat Feb 10, 2016

abock force-pushed the system-object-from-comp-corlib branch from 406e5c9 to 247b4a8 Compare February 10, 2016 06:20

abock force-pushed the system-object-from-comp-corlib branch from 247b4a8 to 9764f6f Compare February 10, 2016 07:05

abock force-pushed the system-object-from-comp-corlib branch from 9764f6f to 37882f8 Compare February 10, 2016 23:03

AlekseyTs reviewed Feb 10, 2016
View reviewed changes

tmat closed this Feb 12, 2016

tmat reopened this Feb 12, 2016

dnfclas added the cla-already-signed label Feb 12, 2016

abock closed this Feb 16, 2016

abock reopened this Feb 16, 2016

dnfclas added the cla-already-signed label Feb 16, 2016

davkean added the Area-Compilers label Mar 8, 2016

abock force-pushed the system-object-from-comp-corlib branch from f2a44b3 to 3a0434d Compare May 12, 2016 23:24

Tests: fix minasync Task<T> to derive from Task

f7aa307

abock force-pushed the system-object-from-comp-corlib branch from 3a0434d to ffc52fa Compare June 1, 2016 01:13

abock added 2 commits May 31, 2016 21:35

Tests: provide MinAsyncCorlibRef

3fa5910

This combines the async features of minasync with mincorlib to produce a minimum unversioned corlib with async stubs.

abock force-pushed the system-object-from-comp-corlib branch from ffc52fa to 9b206e6 Compare June 1, 2016 01:35

MattGertz added the Approved to merge label Jun 2, 2016

tmat merged commit c073826 into dotnet:master Jun 2, 2016

tmat mentioned this pull request Jun 2, 2016

Compilation: use System.Object from target corlib (#8507) #11708

Merged

This was referenced Feb 26, 2017

Script compilations created from a previous compilation have incorrect return type #17403

Closed

Ensure script compilations properly inherit return type from first compilation #17404

Merged

abock mentioned this pull request Jul 17, 2017

Globals and return types for script compilations should be surfaced as ITypeSymbol, not System.Type #20920

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compilation: use System.Object from target corlib #8507

Compilation: use System.Object from target corlib #8507

abock commented Feb 9, 2016

davkean commented Feb 10, 2016

davkean commented Feb 10, 2016

abock commented Feb 10, 2016

davkean commented Feb 10, 2016

abock commented Feb 10, 2016

abock commented Feb 10, 2016

abock commented Feb 10, 2016

AlekseyTs Feb 10, 2016

tmat Feb 11, 2016

AlekseyTs Feb 11, 2016

tmat Feb 11, 2016

tmat commented Feb 11, 2016

davkean commented Feb 11, 2016

tmat commented Feb 11, 2016

tmat commented Feb 12, 2016

tmat commented Feb 12, 2016

abock commented Feb 16, 2016

tmat commented May 12, 2016

abock commented May 12, 2016

tmat commented May 12, 2016

abock commented May 13, 2016

jasonmalinowski commented May 13, 2016

tmat commented May 13, 2016

abock commented Jun 1, 2016

davkean commented Jun 1, 2016

davkean commented Jun 1, 2016

tmat commented Jun 1, 2016

tmat commented Jun 1, 2016

AlekseyTs commented Jun 1, 2016

MattGertz commented Jun 2, 2016

tmat commented Jun 2, 2016

davkean commented Jun 2, 2016

abock commented Jun 2, 2016

Compilation: use System.Object from target corlib #8507

Compilation: use System.Object from target corlib #8507

Conversation

abock commented Feb 9, 2016

davkean commented Feb 10, 2016

davkean commented Feb 10, 2016

abock commented Feb 10, 2016

davkean commented Feb 10, 2016

abock commented Feb 10, 2016

abock commented Feb 10, 2016

abock commented Feb 10, 2016

AlekseyTs Feb 10, 2016

Choose a reason for hiding this comment

tmat Feb 11, 2016

Choose a reason for hiding this comment

AlekseyTs Feb 11, 2016

Choose a reason for hiding this comment

tmat Feb 11, 2016

Choose a reason for hiding this comment

tmat commented Feb 11, 2016

davkean commented Feb 11, 2016

tmat commented Feb 11, 2016

tmat commented Feb 12, 2016

tmat commented Feb 12, 2016

abock commented Feb 16, 2016

tmat commented May 12, 2016

abock commented May 12, 2016

tmat commented May 12, 2016

abock commented May 13, 2016

jasonmalinowski commented May 13, 2016

tmat commented May 13, 2016

abock commented Jun 1, 2016

davkean commented Jun 1, 2016

davkean commented Jun 1, 2016

tmat commented Jun 1, 2016

tmat commented Jun 1, 2016

AlekseyTs commented Jun 1, 2016

MattGertz commented Jun 2, 2016

tmat commented Jun 2, 2016

davkean commented Jun 2, 2016

abock commented Jun 2, 2016