Skip to content

[VS] Scripting fixes - #11643

Merged
dsyme merged 16 commits into
dotnet:mainfrom
TIHan:vs-script-fixes
Jun 25, 2021
Merged

[VS] Scripting fixes#11643
dsyme merged 16 commits into
dotnet:mainfrom
TIHan:vs-script-fixes

Conversation

@TIHan

@TIHanTIHan commented Jun 7, 2021

Copy link
Copy Markdown
Contributor

We have a few issues with F# scripts getting re-analyzed when their dependencies have been updated.

The idea here is to create a VS project that tracks the dependencies of the script so Roslyn can accurately re-analyze it.

lock gate (fun () ->
match files.TryRemove(document.FilePath) with
| true, (projectContext, _) ->
projectContext.Dispose()

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We should not dispose of a projectContext if it's being used as a dependency for another project.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note this TODO

@TIHan

TIHan commented Jun 9, 2021

Copy link
Copy Markdown
ContributorAuthor

It would be preferred we finish #11588 first as it will tie into how scripts will be updated.

@dsyme

Copy link
Copy Markdown
Contributor

So how do we get this under test? :)

@dsyme

Copy link
Copy Markdown
Contributor

@TIHan Can we get this in now the refactoring PR is done?

@TIHan

Copy link
Copy Markdown
ContributorAuthor

I'll see if we can now. I need to think about this as it might change because of the in-memory-documents PR.

@TIHanTIHan changed the title [VS][WIP] Scripting fixes[VS] Scripting fixesJun 25, 2021

let _miscFileService = FSharpMiscellaneousFileService(workspace, miscFilesWorkspace, projectContextFactory)

let filePath1 =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can be done later - but best to create an IDisposable thingy for these

@dsyme

Copy link
Copy Markdown
Contributor

It would be good to get further tests added where we check that re-analysis happens when

  1. a #r referenecd DLL is added and removed+added

  2. a #r "nuget: .." package is deleted and re-restored (do we expect to handle this?)

  3. a type provider invalidation occurs

I'm particularly concerned about the last one as I don't currently understand how Roslyn could know that it needs to do a re-analysis in this case.

@dsymedsyme closed this Jun 25, 2021
@dsymedsyme reopened this Jun 25, 2021
@dsyme

Copy link
Copy Markdown
Contributor

I will merge this, as the fixes are important and the core testing added, and start using it myself.

@TIHan Can you look at the above testing cases?

@dsyme
dsyme merged commit a891d06 into dotnet:mainJun 25, 2021
@TIHan

Copy link
Copy Markdown
ContributorAuthor

Yes I will look at the test cases and get them in today.

Sign up for freeto 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.

2 participants

@TIHan@dsyme