Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't suppress missing FSI transitive references #626

Closed
wants to merge 1 commit into from
Closed

Don't suppress missing FSI transitive references #626

wants to merge 1 commit into from

Conversation

dungpa
Copy link
Contributor

@dungpa dungpa commented Sep 9, 2015

Close #509.

Use throwOnError option so that missing reference exception is propagated correctly to the top level.

@dungpa
Copy link
Contributor Author

dungpa commented Sep 9, 2015

I reproduced the problem using the exact repro by @dsyme in #509.

Should I add it as a unit test? Since it requires adding a few big assemblies, what is a natural place to add the test?

@dsyme
Copy link
Contributor

dsyme commented Sep 10, 2015

LGTM. (I'd personally be willing to accept this change without a test case, since it's most definitely an improvement over the old terrible error message)

@dungpa
Copy link
Contributor Author

dungpa commented Sep 14, 2015

@latkin @KevinRansom @NumberByColors Do you guys have any opinion?

typT |> nonNull "convTypeRefAux"
| ILScopeRef.Module _
| ILScopeRef.Local _ ->
let typT = Type.GetType(qualifiedName,true)
let typT = Type.GetType(qualifiedName, throwOnError=true)
typT |> nonNull "convTypeRefAux"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you still need the nullcheck when using : Type.GetType(qualifiedName, throwOnError=true)

https://msdn.microsoft.com/en-us/library/c5cf8k43(v=vs.110).aspx

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might not be needed anymore. Here is the confusing bit from the link you cited:

GetType only works on assemblies loaded from disk. If you call GetType to look up a type defined in a dynamic assembly defined using the System.Reflection.Emit services, you might get inconsistent behavior. The behavior depends on whether the dynamic assembly is persistent, that is, created using the RunAndSave or Save access modes of the System.Reflection.Emit.AssemblyBuilderAccess enumeration. If the dynamic assembly is persistent and has been written to disk before GetType is called, the loader finds the saved assembly on disk, loads that assembly, and retrieves the type from that assembly. If the assembly has not been saved to disk when GetType is called, the method returns null. GetType does not understand transient dynamic assemblies; therefore, calling GetType to retrieve a type in a transient dynamic assembly returns null.

Reading the source, I'm not entirely sure the method never returns null.

http://referencesource.microsoft.com/#mscorlib/system/reflection/assembly.cs,1454

So to be on the safe side, I still left the null check there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree about the nullcheck ... that is certainly a messed up API.

@latkin
Copy link
Contributor

latkin commented Sep 16, 2015

LGTM

@latkin latkin closed this in 3dd2d62 Sep 16, 2015
@latkin latkin added the fixed label Sep 16, 2015
@latkin latkin added this to the F# 4.0 Update 1 milestone Sep 16, 2015
@dungpa dungpa deleted the missing-fsi-ref branch September 17, 2015 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants