-
Notifications
You must be signed in to change notification settings - Fork 780
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 3033 - make project references known to Roslyn (note: requires Roslyn fix) #3499
Conversation
@cartermp @Pilchie @KevinRansom This is a fairly fundamental set of improvements to the F# + Roslyn integration. With this, the CPSProject now knows about project references, and updates those properly as the project evolves under editing The features in the linked bug depend on this, but it is possible that some other things do as well. As with any changes in LanguageService.fs we should do lots of manual testing. I've done a fair bit, but more would be great. What I've done is approximately:
I haven't done much on
|
My greatest fear in this kind of work is that our use of Roslyn internals will trigger some Roslyn "FailFast" condition, bringing down the VS process. I'd really prefer it if Roslyn never used FailFast anywhere in its codebase |
I'm not sure if this is helpful, but I'm testing this out and I just got this FailFast error: https://gist.github.com/dsyme/f4b966c0a138a7f5161272dd565170ce |
@dhwed AAAARGH yes, I am getting a similar FailFast https://gist.github.com/dsyme/efa645c1c760845d22511e5ec353df37. It is SOOOOO frustrating that Roslyn uses ANY FailFast calls - it is just like a nuclear bomb. No one should EVER insert such calls into production interactive systems |
Looks related to project dependencies:
|
@Pilchie I've marked this as WIP until we work out the required Roslyn fix - and I think we need to scrub Roslyn a bit too to make sure we don't crash into these FailFast cases Repro
The problem is that this code is now being activated when analyzing C# code that references an F# project: The Roslyn project tracker now knows about some F# project (I think it has done for some time?) - and presumably now also knows about the transitive project references of that F# project |
@dsyme Generally, Isn't Fail-Fast is considered a good design practice? Maybe you should suggest them with alternatives that still allow them to find errors in a fast way ;). I have some personal experience with this so I tend to agree with you that If the IDE crashes they get a dump from the IDE, maybe that is one of the reasons? AFAIK This is impossible to do via code (as other threads might continue and important state might change until data is collected) |
@matthid FailFast is not appropriate when a minor IDE feature fails to compete some logic. Even Roslyn analyzers report errors via the diagnostics channel in VS rather than bringing down all of VS |
@Pilchie The problem is this use of a |
@Pilchie Roslyn bug is here: dotnet/roslyn#21719 Should we be making sure |
Looping in @jasonmalinowski and @dpoeschl. I'm on vacation for the next couple of weeks (actually about to head to the airport for a flight to London...) |
Right now only C# and VB projects will have non-null compilations. It's expected that (somewhere) the appropriate call to Project.SupportsCompilation is done to ensure that we don't see precisely this. Looking at the stack above I thought we had already fixed this one. @CyrusNajmabadi can you help look? |
@Pilchie If you want a beer when you land let me know :) @jasonmalinowski @CyrusNajmabadi Systematically forcing a check by changing the return type of GetCompilationAsync in some way might make sense. Not sure if it affects other returns though |
Unfortunately, as it is a public API, we can't do that without a major breaking change. |
@CyrusNajmabadi ok, thanks :) |
I don't think typescript ever ends up in these situations as the "HasReferenceTo" check will always be false as there's no such thing as a C#->TS or TS->C# reference. We're likely only hitting htis for the first time since we're not validating F#->C# or C#->F# references. |
BTW it looks like we're only hitting the above problem when we have an InternalsVisibleTo reference from a C# project to an F# project - the Visual F# IDE Tools build uses that. That scenario must be quite rare - though not 100% so - and I might modify this PR to workaround that case so we can accept it in some form and get it out in nightlies. I'm not quite sure how to workaround it though short of hardwiring a specific assembly name (VisualFSharp.ProjectSystem.Base) @dhwed Did you hit this case when using VisualFSharp.sln? Or your own solution? It could be there are other places lurking where we need to check for proejct.SupportsCompilation (though I'm sure @CyrusNajmabadi will do a complete scrub in any case :) ) |
Easiest would be if you can provide me with a repro solution. I can then try to scrub out any additional cases that may hit this sort of thing. |
@dsyme I found this in my own solution, and we do have a bunch of |
@CyrusNajmabadi VisualFSharp.sln hits it, see the repro in the linked bug. It's a bit of a PITA though since you have to build a version of the F# tools and then use it |
@cartermp @KevinRansom @Pilchie @brettfo I've removed the WIP tag as I believe this is sound apart from the required fix to Roslyn Unfortunately I don't think we'll be able to take these fixes before we know that the VS we are installing into contains an update to the Roslyn bits with the linked fix. Can I leave it to you guys to work out the details about when we can pull it? thanks |
override this.CompilationOptions = checkOptions.OtherOptions | ||
override this.CompilationReferences = | ||
checkOptions.OtherOptions | ||
|> Array.choose (fun flag -> if flag.StartsWith("-r:") then Some flag.[3..] else None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can references not start with /r:
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thx.
updated <- true | ||
|
||
// Update the metadata/project references in projectContext. Note that AbstractProject.cs converts metadata references | ||
// to project references automagically by consulting the project tracker. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the referenced project hasn't been setup yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the referenced project hasn't been setup yet?
TBH I don't fully understand the Roslyn code that does the metadata --> project reference conversion, but my impression is that it would handle this case. Certainly the project references were in place on initial project load - which must encounter this
@dsyme has this made any changes to solution load performance (particularly Dense.sln)? |
FYI - this will be targeted for 15.5 for now. |
@cartermp Hmmm ... we had trouble getting the error list fix into 15.3 when a roslyn fix was also needed there, and ended up delaying that until just after I think the underlying reason was that we weren't doing sufficiently proactive validation of the vs2017-rtm release branch via nightlies etc., just relying on the previews, where the turnaround was still a bit long I'm not sure what to do about this - obviously channels for nightlies is the real solution. But it would be great to at least get this into the first preview of 15.5 thx |
Would this have any negative impact if it were merged into master? Prior to the first preview of 15.5, we'll likely do a merge of master --> vs2017-rtm, and only cherry-pick from there on out. If this is in master prior to that, there won't be any issues. |
Yes, if we use with Roslyn binaries in 15.3 then we get crashes, i.e. we would gt crashes when using VisualFSharp.sln + nightlies This can safely go into vs2017-rtm (i.e. the release branch for 15.5 - we should really use different branch names for each release train?), after validation and systematic use against previews of 15.5 and/or local build+install of roslyn master |
Okay. In that case, let's merge this into As for the naming, @brettfo (on vacation right now) wants to do something like In the long run, we may want a better overall story for how we realize the concurrent releases of VS with our own github repo. Since nightlies are always for the latest stable release, and we just want the OSS contribution story to be "put it into master", I'd caution against changing what we have today...it certainly makes things a bit more challenging for @brettfo, but at least the majority of PRs can come in and the contribution process is pretty painless w.r.t which branch to commit against. |
@cartermp Thanks :) |
The Roslyn fix (dotnet/roslyn#21721) is confirmed as going in for 15.5. @Pilchie / @brettfo Can we verify that we won't be pulling anything else in for 15.4? I'd like to get this in so we can start verifying that this + the Roslyn work addresses #3033. |
@cartermp @KevinRansom Should we merge both this and #3564 into a 15.5 branch? I'm very keen to get this into the same stream as #3564 - which I presume is targeting 15.5?, so we don't have to revalidate both all over again. There are important fixes here which are related to fixes in #3564 and could interact with them |
Yes, let's do that. Both are targeting 15.5. |
@dsyme I resolved the conflicts with master. Can you check that out. I will merge this one into the insertion branch, once I have added merged master there. |
@KevinRansom The code looks fine. However AFAIK the required Roslyn fix is only in 15.5, so be careful we don't insert this into 15.4? |
@dsyme We're past the window for insertions like this into 15.4 now. |
merge #3499 into vs2017-rtm
Note: Requires Roslyn fix dotnet/roslyn#21721
Fix for #3033
There were a few problems
In the old project system code, project references were not being reported via
IProjectSite
correctly, so when syncing'ingIProjectSite --> projectContext
we weren't reporting any metadata references coming from project referencesThe Roslyn AbstractProject.cs code is meant to automagically convert metadata references to project references. However, this only works if the
BinOutputPath
is set correctly in the projectContext. THis means this value must be reported byIProjectSite