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

Fix #492 - use ConvILTypeRefUnadjusted not ConvILTypeRef #510

Closed
wants to merge 1 commit into from

Conversation

latkin
Copy link
Contributor

@latkin latkin commented Jun 18, 2015

  • Bug number Runtime error with SqlEntityConnection TP under VS 2015 RC and F# 3.1 #492
  • Customer scenario Runtime crash in user code (dynamic assembly load failure) due to bad codegen. This is a regression. Occurs when downtargeting to F# 3.1 and using any query expression against the SQL Entity type provider (or other type providers that use static linking in a similar way).
  • Fix description

Use correct API when encoding quoted assembly references. ConvILTypeRefUnadjusted properly handles the case where the reference is statically linked. ConvILTypeRef does not.

These two used to be a single API. They were split in 640db00 and it was a simple oversight that the wrong one was wired up here.

  • Testing done
    We have private tests which cover these type providers, they were simply never run in a cross-version configuration. That's frustrating, as cross-version testing was automated recently with Automated cross-targeting testing #446. Running those tests, the bug would have been found a month ago. With the fix, they all pass.

@@ -967,7 +967,7 @@ and ConvTyconRef cenv (tcref:TyconRef) m =
| TProvidedTypeExtensionPoint info when not cenv.g.isInteractive && not info.IsErased ->
// Note, generated types are (currently) non-generic
let tref = ExtensionTyping.GetILTypeRefOfProvidedType (info.ProvidedType, m)
ConvILTypeRef cenv tref
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dsyme there is now only one direct use of ConvILTypeRef, below on line 978. Can you take a second look and confirm that this call is correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

There would certainly be no harm in adjusting that one for uniformity, but its harmless either way - the case is only used for the built-in primitive types like these: https://github.com/Microsoft/visualfsharp/blob/fsharp4/src/fsharp/FSharp.Core/prim-types-prelude.fsi#L73 as mentioned here https://github.com/Microsoft/visualfsharp/blob/275b832e9dd1a4bd64ed3accd218384b901be1d2/src/fsharp/tast.fs#L3681. I think this specific case is probably only used for nativeptr if at all.

@latkin
Copy link
Contributor Author

latkin commented Jun 19, 2015

This guy has also been approved

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.

4 participants