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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,5 @@ tests/fsharpqa/Source/CodeGen/EmittedIL/ComputationExpressions/ComputationExprLi
*.chk
*.bak
*.orig
*.mdf
*.ldf
4 changes: 2 additions & 2 deletions src/absil/ilreflect.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,11 +336,11 @@ let convTypeRefAux (cenv:cenv) (tref:ILTypeRef) =
| None ->
let asmName = convAssemblyRef asmref
FileSystem.AssemblyLoad(asmName)
let typT = assembly.GetType(qualifiedName)
let typT = assembly.GetType(qualifiedName, throwOnError=true)
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.



Expand Down