Skip to content

Fix lexer cache #2158

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.

Sign up for GitHub

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

Jump to bottom
Merged
KevinRansom merged 5 commits into from
Jan 6, 2017
Merged

Fix lexer cache #2158

KevinRansom merged 5 commits into from
Jan 6, 2017

Conversation

@vasily-kirichenko
Copy link
Contributor

@vasily-kirichenko vasily-kirichenko commented Jan 4, 2017

This fixes #2155

It reverts #2090, then fixes the OutOfRangeException in getCachedSourceLineData.

@vasily-kirichenko vasily-kirichenko mentioned this pull request Jan 4, 2017
Closed
@vasily-kirichenko
Copy link
Contributor Author

vasily-kirichenko commented Jan 4, 2017

OMG, this code is... let's call it fragile. Please, don't touch it ever in the future.

@vasily-kirichenko vasily-kirichenko mentioned this pull request Jan 4, 2017
Closed
@cartermp
Copy link
Contributor

cartermp commented Jan 4, 2017

Perhaps in the future we'll be able to do away with all of these +1/-1 offsets and make FCS 0-based...

@dsyme
Copy link
Contributor

dsyme commented Jan 4, 2017

Perhaps in the future we'll be able to do away with all of these +1/-1 offsets and make FCS 0-based...

  1. Always use these helpers to convert. Never put an explicit +1/-1 on a line number https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/range.fsi#L104

  2. Perhaps in debug mode enable CHECK_LINE0_TYPES, see https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/range.fsi#L90, though it's quite invasive on client code as you have to strip the annotations out when passing to Roslyn. Still, it helps find bugs

@vasily-kirichenko
Copy link
Contributor Author

vasily-kirichenko commented Jan 4, 2017

@dsyme Maybe rename fromZ and toZ to fromVS and toVS? And ZeroBasedLineAnnotation to VSLineNumber?

@smoothdeveloper
Copy link
Contributor

smoothdeveloper commented Jan 4, 2017

@vasily-kirichenko this stuff is in FCS / not specific to VS and I think if it was to be renamed fromZero and toZero would be the correct names.

@vasily-kirichenko
Copy link
Contributor Author

vasily-kirichenko commented Jan 4, 2017

@smoothdeveloper As there are other zero line based editors, then yes, the functions should keep their names, but we should create aliases in FSharp.Editor project with the proposed names.

@KevinRansom
Copy link
Contributor

KevinRansom commented Jan 5, 2017

@dotnet-bot test this please

@KevinRansom
Copy link
Contributor

KevinRansom commented Jan 5, 2017

@vasily-kirichenko

1) Failed : Microsoft.VisualStudio.FSharp.Editor.Tests.Roslyn.GoToDefinitionServiceTests.VerifyDefinition
Incorrect information returned for fileContents=<<<
type TestType() =
    member this.Member1(par1: int) =
        printf "%d" par1
    member this.Member2(par2: string) =
        printf "%s" par2

[<EntryPoint>]
let main argv =
    let obj = TestType()
    obj.Member1(5)
    obj.Member2("test")>>>, caretMarker=<<<obj.Member1>>>, expected =<<<Some (3, 3, 16, 23)>>>, actual = <<<<null>>>>
at Microsoft.VisualStudio.FSharp.Editor.Tests.Roslyn.GoToDefinitionServiceTests.VerifyDefinition() in D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\tests\unittests\GoToDefinitionServiceTests.fs:line 108

2) Failed : Microsoft.VisualStudio.FSharp.Editor.Tests.Roslyn.QuickInfoProviderTests.ShouldShowQuickInfoAtCorrectPositions
  Expected: <Some(val x : int
Full name: Test.x)>
  But was:  null
at Microsoft.VisualStudio.FSharp.Editor.Tests.Roslyn.QuickInfoProviderTests.ShouldShowQuickInfoAtCorrectPositions() in D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\tests\unittests\QuickInfoProviderTests.fs:line 105

Run Settings
    RuntimeFramework: V4.0
    RunAsX86: True
    WorkDirectory: D:\j\workspace\release_ci_pa---3f142ccc\release\net40\bin
    MaxAgents: 1
    NumberOfTestWorkers: 1
    Verbose: True

Test Run Summary
    Overall result: Failed
   Tests run: 2936, Passed: 2934, Errors: 0, Failures: 2, Inconclusive: 0
     Not run: 99, Invalid: 0, Ignored: 99, Explicit: 0, Skipped: 0
  Start time: 2017-01-05 07:14:39Z
    End time: 2017-01-05 07:31:22Z
    Duration: 1003.057 seconds

@vasily-kirichenko
Copy link
Contributor Author

vasily-kirichenko commented Jan 5, 2017

@KevinRansom fixed.

KevinRansom
KevinRansom approved these changes Jan 6, 2017
View reviewed changes
vsintegration/src/FSharp.Editor/Common/CommonHelpers.fs
while i > 0 && (match sourceTextData.[i] with Some data -> not (data.IsValid(lines.[i])) | None -> true) do
i <- i - 1
i
// Rescan the lines if necessary and report the information
Copy link
Contributor

@KevinRansom KevinRansom Jan 6, 2017

Choose a reason for hiding this comment

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

scanStartLine made my head explode or at least the while loop condition did.
I don't know whether this is any more readable though:

             let scanStartLine =
                 let rec scanStart i =
                     if i > 0 && (Option.isNone (sourceTextData.[i]) || not (sourceTextData.[i].Value.IsValid(lines.[i]))) then scanStart (i - 1)
                     else i
                 scanStart startLine

Copy link
Contributor Author

@vasily-kirichenko vasily-kirichenko Jan 6, 2017

Choose a reason for hiding this comment

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

I'm 100% with you on this. It must be covered by tests, then rewritten in a more functional and safe manner. The number of things this algorithm operates is not as huge to make the code so imperative.

@KevinRansom KevinRansom merged commit c2821eb into dotnet:master Jan 6, 2017
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
@vasily-kirichenko @KevinRansom
Fix lexer cache (dotnet#2158)
5dc5887 
* revert "Use Roslyn line numbers only in lexer cache" dotnet#2090

* fix OutOfRangeException in getCachedSourceLineData

* fixed: lexer cache does not work at all

* fix finding start line for relexing in getCachedSourceLineData
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@KevinRansom KevinRansom KevinRansom approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Very wrong type provider usage syntax highlighting

6 participants

@vasily-kirichenko @cartermp @dsyme @smoothdeveloper @KevinRansom @msftclas