Skip to content

Conversation

@0101
Copy link
Contributor

@0101 0101 commented Mar 14, 2023

This should fix issues with namespaces disappearing with in-memory cross-project references, which happened quite often with FCS solution when using live buffers (#14033).

The underlying issue was in incremental builder where it's assumed that computing the last bound model would evaluate all the other ones too, which is not necessarily the case, and then accessing them by ValueOption.Value.

For some reason the diagnostics for this didn't show. They are reported from here:

try
return! tcImports.TryRegisterAndPrepareToImportReferencedDll(ctok, nm)
with e ->
errorR (Error(FSComp.SR.buildProblemReadingAssembly (nm.resolvedPath, e.Message), nm.originalReference.Range))
return None

One suspected location which could produce an "unlinked" graph node is here:

boundModels = state.boundModels.SetItem(slot, GraphNode(node.Return newBoundModel))

Even though we already have the result, the internal cachedResult will not be automatically populated and tryPeekValue will return None. Will probably fix that in a separate PR.

But this doesn't have to be (the only) reason for the issue, so better just avoid using .Value altogether. Probably should get rid of this one as well:

let tcInfo, _ = fullGraphNode.TryPeekValue().Value

@0101 0101 requested a review from a team as a code owner March 14, 2023 11:40
Copy link
Contributor

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Tests? 🤔

@0101
Copy link
Contributor Author

0101 commented Mar 14, 2023

Tests? 🤔

Wasn't able to figure out exact repro steps that could be put into a test...

If existing tests still pass, we're replacing an unsafe call to .Value with a match, should be an improvement 😄

@majocha
Copy link
Contributor

majocha commented Mar 14, 2023

Wasn't able to figure out exact repro steps that could be put into a test...

I've seen it throw exception exactly once when running VS in debug mode. I thought Option.Value a bit sus 🤨 but maybe a fluke because of changing repo branches or something. Not easy to hit.

@vzarytovskii vzarytovskii enabled auto-merge (squash) March 14, 2023 15:47
@vzarytovskii vzarytovskii merged commit ea5a144 into dotnet:main Mar 14, 2023
vzarytovskii pushed a commit that referenced this pull request Mar 14, 2023
* Cache parsing results in incremental build (#14852)

* First take on the F# telemetry (#14889)

* Array.Parallel - search functions (tryFindIndex,tryFind,tryPick) (#14827)

* Searching functions for Array.Parallel added

* with [<Experimental("Experimental library feature, requires '--langversion:preview'")>] annotation to give us space to change/remove this API in the future if needed

* Add `GraphNode.FromResult` (#14894)

* Add GraphNode.FromResult

* fantomas

* Fix missing reference (#14892)

* Fix missing reference

* undo whitespace change

---------

Co-authored-by: Jakub Majocha <[email protected]>
Co-authored-by: Petr <[email protected]>
Co-authored-by: Tomas Grosup <[email protected]>
Co-authored-by: Petr Pokorny <[email protected]>
Co-authored-by: Kevin Ransom (msft) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants