Skip to content
This repository has been archived by the owner on Oct 31, 2021. It is now read-only.

WIP update to FCS 10 #1496

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

smoothdeveloper
Copy link
Contributor

changes to get solution to compile with FCS 10

notes:

  • I've fixed framework >= net40 in paket, this pulls less things and keep project files tidier
  • new FCS made IsResultObsolete internal, it was used once but oddly we don't seem to use the callback we are giving there anywhere else
  • call to ParseAndCheckFileInProject changed, I haven't checked what was the null parameter we were passing, using explicit parameter names for this call now

Caution I haven't run those bits yet, just pushing the changes to get it compiling.

notes:

* I've fixed framework >= net40 in paket, this pulls less things and keep project files tidier
* new FCS made IsResultObsolete internal, it was used once but oddly we don't seem to use the callback we are giving there anywhere else
* call to ParseAndCheckFileInProject changed, I haven't checked what was the null parameter we were passing, using explicit parameter names for this call now
@smoothdeveloper
Copy link
Contributor Author

Looking into failing test, there is SymbolClassifierTests with seemingly moved ranges.

My VS SDK stuff is apparently broken, trying to fix it to test debugging the branch.

@vasily-kirichenko
Copy link
Contributor

If the tests check Printf specifiers, then yes, the ranges were wrong and now they fixed - start column is shifted to one position to left, so you should remove something like (printfRange.StartColumn + 1) in the classifier.

@smoothdeveloper
Copy link
Contributor Author

Thanks, I'll fix those tests accordingly.

@smoothdeveloper
Copy link
Contributor Author

I'm getting warning on build:

Warning MSB3243: No way to resolve conflict between "Microsoft.VisualStudio.Threading, Version=14.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" and "Microsoft.VisualStudio.Threading, Version=12.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a". Choosing "Microsoft.VisualStudio.Threading, Version=14.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" arbitrarily. (1, 1)

Not sure if it is something to be concerned about.

Running the extension, I'm seeing exception (leading to crash) in TaskListCommentExtractor:

image

image

I vaguely remember something about lexer cache being broken during VS2017 development, any clue about that?

@vasily-kirichenko
Copy link
Contributor

Just remove that List Comment extractor feature altogether. I've know nobody who used it.

@smoothdeveloper
Copy link
Contributor Author

I fixed the tests regarding printf formatting, now searching for the correct place in the code as it highlights 1 column more:

image

I'm looking at https://github.com/fsprojects/VisualFSharpPowerTools/blob/master/src/FSharp.Editing.VisualStudio/Symbol/PrintfSpecifiersUsageTagger.fs but don't see obvious location

@latkin by chance do you know the place I'd need to adjust the tagging?

@smoothdeveloper smoothdeveloper force-pushed the update-fcs-10 branch 2 times, most recently from 6c7f482 to 6a18239 Compare February 11, 2017 15:52
@smoothdeveloper
Copy link
Contributor Author

I'm getting issues related to breaking changes in FCS:

System.MissingMethodException : Method not found: 'Void Microsoft.FSharp.Compiler.SourceCodeServices.FSharpProjectOptions..ctor(System.String, System.String[], System.String[], System.Tuple2<System.String,Microsoft.FSharp.Compiler.SourceCodeServices.FSharpProjectOptions>[], Boolean, Boolean, System.DateTime, Microsoft.FSharp.Core.FSharpOption1<Microsoft.FSharp.Compiler.SourceCodeServices.UnresolvedReferencesSet>)'.

I think we need to have FSharpLint & Fantomas updates to FCS 10.

@vasily-kirichenko
Copy link
Contributor

Yes, FSharpLint and Fantomas must be update as well, it's a known issue.

@cloudRoutine
Copy link
Contributor

cloudRoutine commented Feb 11, 2017

@smoothdeveloper fsharp/fsharp-compiler-docs#696 this is going to hold you up

@smoothdeveloper
Copy link
Contributor Author

@vasily-kirichenko thanks, somehow I tried that already but I'm still having the same behaviour.

If that's the last bug remaining after I complete the fixes, I think we could just keep that as a known defect for that release until we find the culprit.

I've made a local build of Fantomas with latest FCS and going to give that new version a try for a short while.

@vasily-kirichenko
Copy link
Contributor

thanks, somehow I tried that already but I'm still having the same behaviour.

Have you tried to debug it? We should not publish a new release having such a defect unfixed,

@smoothdeveloper
Copy link
Contributor Author

@vasily-kirichenko I'm trying now and it seems to solve the issue, I don't know what was up with CTRL-F5 but it seems it wouldn't deploy new code for some reason.

@latkin
Copy link
Contributor

latkin commented Feb 14, 2017

The printf feature has definitely not been broken for the past ~year or whenever it was debuted.

So why does it now have an off-by-1 bug? Maybe there is a regression in the range reporting?

, fileversion = 0
, source = source
, options = options
, textSnapshotInfo = (*IsResultObsolete*) (fun _ -> isResultObsolete filePath)
Copy link
Contributor

Choose a reason for hiding this comment

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

THis should be textSnapshotInfo = null?

@dsyme
Copy link
Contributor

dsyme commented Feb 17, 2017

So why does it now have an off-by-1 bug? Maybe there is a regression in the range reporting?

There was some +1/-1 change to the code when it got integrated into the Visual F# Tools. I don't fully understand what it was. I've just assumed it's now working in the Visual F# Tools and in FCS we fixed up a bunch of tests to match

@smoothdeveloper
Copy link
Contributor Author

@latkin thanks, actually this is fixed in both FCS 10 & VFPT in this PR now, the - 1 have been tamed.

In my experience I find it better to add a bit of verbose comments about those things, but my issue was mainly about changing the code and seeing the impact in VS instance, sometimes a ctrl+F5 doesn't do the job etc.

@dsyme thanks for the review, yes I'll push an update soon with this textSnapshotInfo being removed, the PR has been on hold until new release of Fantomas is published on nuget (@dungpa, would you review the fsprojects/fantomas#206 PR?).

I'm using succesfully the PR in VS2015 with my day job solution, it seems to help a bit staying at 2.3gb instead of reaching the 2.6/2.8gb borderline, this is with resharper and mid size solution of about 40 mixed languages projects, if we could trim memory used by assembly references this would help to stay even longer in safe/stable zone.

@smoothdeveloper
Copy link
Contributor Author

I'm looking at failing tests related to gotodefinition, they seem to timeout, when I debug I reach https://github.com/fsprojects/VisualFSharpPowerTools/blob/64b407050a2e56fa109766f2b7464ee94b4717e0/tests/FSharp.Editing.VisualStudio.Tests/Mocks.fs#L291 where projectItem is null.

Any idea what is causing this in our tests? When I try similar go to definition, for example on System.IO.File, I'm getting properly redirected (to referencesource.microsoft.com).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants