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

15.3 preview 3: FCS does not respect file ordering defined in .NET Core project files #3260

Closed
vasily-kirichenko opened this issue Jun 27, 2017 · 21 comments
Labels
Area-LangService-API Bug Impact-High (Internal MS Team use only) Describes an issue with extreme impact on existing code.
Milestone

Comments

@vasily-kirichenko
Copy link
Contributor

image

Code completion, tooltips, etc. don't work either.

@rodrigovidal
Copy link

Same here

@vasily-kirichenko
Copy link
Contributor Author

I checked it on both .NET Core 1.0.4 and .NET Core 2.0 preview 2: the same behavior.

@saul
Copy link
Contributor

saul commented Jun 27, 2017

Absolutely no surprise. Nobody on the Roslyn project team side seemed to care that we didn't have file ordering.

@saul
Copy link
Contributor

saul commented Jun 27, 2017

Also I'm pretty sure all of the issues you've raised for 15.3 should be raised in https://github.com/dotnet/project-system, not here. The code doesn't even live in this repo.

@enricosada
Copy link
Contributor

enricosada commented Jun 27, 2017

@saul it is a surprise.

UI treeview is a thing (minor priority for now, and moved to later, not for sure in 15.3 dotnet/project-system#1875 (comment)).
but this is build (ok vs build not msbuild), how to get fsc args should be the same.

@enricosada
Copy link
Contributor

@vasily-kirichenko Roslyn (https://github.com/dotnet/roslyn) <> Roslyn project system (https://github.com/dotnet/roslyn-project-system).
Name is strange, but because is based on roslyn-workspaces and new CPS, but there is not a lot c# specific there.
collaborating with roslyn-ps, no issue at all. they fixed and refactored a lot for F#

@dotnet dotnet deleted a comment from forki Jun 27, 2017
@dotnet dotnet deleted a comment from realvictorprm Jun 27, 2017
@dotnet dotnet deleted a comment from isaacabraham Jun 27, 2017
@dotnet dotnet deleted a comment from vasily-kirichenko Jun 27, 2017
@cartermp
Copy link
Contributor

@srivatsn Any idea where this comes from?

but this is build (ok vs build not msbuild), how to get fsc args should be the same

Is this the remaining issues with the argument parser?

@cartermp
Copy link
Contributor

@Pilchie Do you think this is related to your comment here? #3266 (comment)

@Pilchie
Copy link
Member

Pilchie commented Jun 27, 2017

@cartermp - no that should be a VS editor thing only. Wouldn't effect build or the solution explorer.

BTW @enricosada and @vasily-kirichenko dotnet/roslyn-project-system was already renamed to just dotnet/project-system.

@srivatsn
Copy link
Contributor

srivatsn commented Jun 27, 2017

Ya the file order in the tree doesn't affect the language service. The project system grabs the command line arguments as they are sent to the compiler (which is correct as evidenced by the build succeeding) and sends that to the language service. Something's going wrong in the language service.

@enricosada
Copy link
Contributor

probably is the same language service bug, as reported in #3266 (comment)
is confirmed doesnt load command line args correctly.

@srivatsn @Pilchie @KevinRansom there is a log setting to enable/sethigher to confirm is a dupe? to see args passed to language service is enough.

@srivatsn
Copy link
Contributor

I just looked at the F# LS code and it seems to be getting all source files\references etc from an IProjectSite from a IProjectSite (which is the interface that the old F# PS implements) instead of getting them from the Roslyn workspaces which would get populated with all this information. I think this needs to be changed to pull from the workspace for CPS-based projects.

@cartermp cartermp added this to the VS 2017 Updates milestone Jun 27, 2017
@cartermp cartermp added the Impact-High (Internal MS Team use only) Describes an issue with extreme impact on existing code. label Jun 27, 2017
@Pilchie
Copy link
Member

Pilchie commented Jun 27, 2017

@srivatsn and I chatted, and we think the easiest way to do this might be to create an implementation of IProjectSite directly in the F# language service code and implement it to ask the Roslyn Project objects for their references and files. This will leave #defines not working right, since they don't get set in the roslyn workspace, but will be a big first step up.

@brettfo Is working on getting the latest F# bits inserted for Preview 4, but can start working on this in parallel I think.

@dsyme
Copy link
Contributor

dsyme commented Jun 27, 2017

This will leave #defines not working right, since they don't get set in the roslyn workspace, but will be a big first step up.

I was wondering about the #defines. Thanks for tracking this

For completeness, some other options are relevant to on-the-fly type checking, notably these (after searching through CompileOps.fs)

  • --o The output assembly name (for InternalsVisibleTo)
  • --warn/nowarn/warnon/warnaserror
  • --codepage (the input code page)
  • --checked (indicates if open FSharp.Operators.Checked is implied)
  • --mlcompatibility (I think never used)

Of these the output assembly name and the warnings are by far the most important

@enricosada
Copy link
Contributor

enricosada commented Jun 27, 2017

@srivatsn @Pilchie as a note, why instead of parse back all the fsc arguments, the language service dont just read the FscCommandLineArgs msbuild items? is already the array splitted with fsc args.. works really well for FSAC in vscode/vim/emacs project parser.

So we dont care anymore about msbuild -> fsc task -> parse back somehow and maintain alignment

There is no way for new IProjectSite to ask roslyn workspaces for that msbuild item list? afaik FCS need just fsc args to initialize it (plus p2p refs, ok)

@srivatsn
Copy link
Contributor

That msbuild item itself is not available at this layer, however that string (the entire command line) get passed to this method - right now, that method no-ops for F# since there's no CommandLineParser. However, we can implement a type that gets that and implements IProjectSite too..

@enricosada
Copy link
Contributor

enricosada commented Jun 27, 2017

However, we can implement a type that gets that and implements IProjectSite too

@srivatsn awesome, afaik (/cc @dsyme @KevinRansom), the F# compiler arguments api is the same of c#, so the string -> IEnumerable<string> splitter can be reused from c#

var splitArguments = CommandLineParser.SplitCommandLineIntoArguments(commandLine, removeHashComments: false);

the rest is pattern match on args..

but is not already implemented in dotnet/project-system#2206 by @brettfo as FSharpParseBuildOptions : IParseBuildOptions? so just missing is that it implements IProjectSite ? or an adapter?

@enricosada
Copy link
Contributor

--o The output assembly name (for InternalsVisibleTo)

CommonCommandLineArguments.OutputFileName?

--codepage (the input code page)

CommonCommandLineArguments.Encoding

--checked (indicates if open FSharp.Operators.Checked is implied)
--mlcompatibility (I think never used)

CompilationOptions. CompilationOptions.Features ?

--warn/nowarn/warnon/warnaserror

CompilationOptions. CompilationOptions.SpecificDiagnosticOptions

@srivatsn
Copy link
Contributor

srivatsn commented Jun 27, 2017

The parser in the project system is basically just enough to understand what's a reference and source file versus what's not. This is so that it can track changes to references and source files individually and incrementally update the language service. If any of the options change, since that's rarer, it simply tells the LS to recreate a compilation. Ideally the Project system wouldn't ever parse and understand the command line and keep it as mostly a passthrough.

That type cannot implement IProjectSite as it's not exposed to the LS in anyway. The contract with the language service is IWorkspaceProjectContext

The reason we want to make sure files\references are added to the workspace and not just parsed into a IProjectSite in the SetOptions call is so that any Roslyn extension that works at the workspace level on documents will also work for F# - If we make everything in terms of IProjectSite then they'll have to know about IProjectSite as well.

@enricosada
Copy link
Contributor

thx for info @srivatsn , really appreciated

@KevinRansom
Copy link
Member

This is addressed by: #3564

@cartermp cartermp modified the milestones: VS 2017 Updates, 15.5 Jun 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-LangService-API Bug Impact-High (Internal MS Team use only) Describes an issue with extreme impact on existing code.
Projects
None yet
Development

No branches or pull requests

10 participants