safesparrow / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
1 stars 0 forks source link

Improve top level auto open detection. #31

Closed nojaf closed 1 year ago

nojaf commented 1 year ago

As we discussed in https://github.com/safesparrow/fsharp/pull/24#discussion_r1035365291. This makes the last resort link to all files scenario stricter and pulls up the content in a module with a namespace prefix scenario.

safesparrow commented 1 year ago

I added some comments, but I think there is a more general issue we're having, which is communication 😅

I'm realising that many of my comments are the result of different assumptions about what the current algorithm aims to do.

For example, I was always convinced that we would end up with a solution that avoids a link if the only reference is an open A.B statement and A.B only contains nested modules. But, as we discussed earlier, this would require a separate piece of information to be tracked, which we would pass into the TcState for each file, so that type-checking doesn't fail on that open A.B statement.

I think we should have a chat about that and other things. It's probable that the above 'optimisation' would be negligible, because how often does someone reference a module but not use its contents? Maybe not too often. That would somewhat justify different handling of namespaces and modules.

Overall, I simply realised I don't really understand the dependency resolution algorithm. Maybe that's fine, but it prevents me from reasoning about it. I guess we just switched roles 😅

safesparrow commented 1 year ago

Unrelated to this PR, but... Given that our TrieNodeInfo allows only one file for a module, how do we handle a situation where the same module is (incorrectly) defined in two places? I guess we will somewhat randomly pick one of the files as a dependency when referencing it? Or does the code throw when trying to merge two modules with the same name, from different files?

nojaf commented 1 year ago

I think there is a more general issue we're having, which is communication

It appears so, although I think we can easily overcome that problem with a chat.

For example, I was always convinced that we would end up with a solution that avoids a link if the only reference is an open A.B statement and A.B only contains nested modules.

This is somewhat the case. Consider: A.fs

namespace X.Y

module A =
   let a = 0

B.fs

namespace X.Y

type B = int

C.fs

module C

open X.Y

Only B will be a dependency of C. A is not exposing itself at the X or Y level of the trie.

However, if we don't have file B: A.fs

namespace X.Y

module A =
   let a = 0

C.fs

module C

open X.Y

A will be a dependency of C because the open X.Y needs to lead to something in the trie. This case is covered by the ghost dependency resolution. After all the content of C.fs was processed, we verify that the state does not have any lingering open statements. open X.Y will not lead to any files in the trie (as A doesn't expose anything at the Y level). To overcome this we look in the child nodes of Y if we can find any index that is higher than the index of C and we pick this index as a dependency to satisfy the open X.Y statement in C.

Or does the code throw when trying to merge two modules with the same name, from different files? No idea what happens in that scenario 🙈. It will probably throw.

nojaf commented 1 year ago

I think this is good to go. Please take a peek at all this. You would need to approve it before I can merge it.