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 #337: skip interfaces-that-lie-outside-the-set-of-referenced-assemblies #356

Closed
wants to merge 3 commits into from

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Apr 10, 2015

When the assembly reference set for the compilation is incomplete, some interfaces to types relevant to compilation may lie in assemblies outside the assembly reference set. This applies particularly to private ("internals visible to") interfaces found in .NET assemblies.

This causes very substantial usability bugs in practice as various parts of type inference and other checking "give up" when you get this condition. The C# compiler doesn't give up in the same way.

In most cases it is reasonable to simply skip interfaces-that-lie-outside-the-set-of-referenced-assemblies during F# compilation. Skipping unresolvable interfaces is pretty much harmless: any substantive analysis on the interface type (such as implementing it) will require the assembly holding the interface type.

There are some exceptions: if an interface I1 lies outside the referenceable set and you try to implement I2 inheriting from I1 then we'd better not skip I1. Indeed if you even try to find the methods on I2 then we'd better not skip I1. These are covered by "FoldPrimaryHierarchyOfType" in the code.

I've tested this manually on the example in #337, though not added testing as part of this fix. Constructing compilations with incomplete reference sets is a bit of a PITA - @latkin, do we have any of these compilations already?

@dsyme dsyme changed the title skip unloadable interfaces Fix #337: skip interfaces-that-lie-outside-the-set-of-referenced-assemblies Apr 10, 2015
@latkin
Copy link
Contributor

latkin commented Apr 10, 2015

There is a long-failing internal IDE test which is due to this issue. The code is basically the same as #337, though -- use types from System.Data, compiler complains about missing IXmlSerializable:

open System
open System.Data

let dataset = new DataSet("test add reference")
Console.WriteLine(dataset.DataSetName)
Console.ReadKey(true)

Besides that, I don't know of any existing tests for these cases.

@KevinRansom
Copy link
Member

@dsyme what is the status of this PR: are we going to be able to add test cases to ensure that the issue?

Thanks

@dsyme
Copy link
Contributor Author

dsyme commented Apr 22, 2015

I think we can try using the "long-disabled IDE test" as the test for this fix. If that now passes then that should be sufficient.

@dsyme
Copy link
Contributor Author

dsyme commented Apr 23, 2015

I added a command-line compilation of the DataSet example as a test. The command-line compiler generates the "you need System.Xml ofr IXmlSerializer" error message.

@vladima
Copy link
Contributor

vladima commented Apr 24, 2015

Out of curiosity what will be the user experience in case like the one below:

let conn: System.Data.SqlClient.SqlConnection = null
conn // type dot after conn

Will intellisense list be displayed and if yes - will we print correctly names of types that reside in non-referenced assemblies - in this example method EnlistTransaction method has parameter of type System.Transactions.Transaction that is defined in System.Transactions?

@dsyme
Copy link
Contributor Author

dsyme commented Apr 24, 2015

I'm pretty sure that will still ask for the reference to be added.

@vladima
Copy link
Contributor

vladima commented Apr 24, 2015

But will it at leash show the intellisense list or it still will be a <NOTE> thing that everybody hate?

@latkin
Copy link
Contributor

latkin commented Apr 27, 2015

@vladima I synced this and tried it out on Friday, you still get the <NOTE> intellisense blocker

@dsyme dsyme closed this in d52230b Apr 27, 2015
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