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

textDocument/codeAction request has end character set as 1.7976931348623157E+308 #80288

Closed
rjmholt opened this issue Sep 3, 2019 · 31 comments
Closed
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug editor-code-actions Editor inplace actions (Ctrl + .) help wanted Issues identified as good community contribution opportunities insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Milestone

Comments

@rjmholt
Copy link

rjmholt commented Sep 3, 2019

From PowerShell/vscode-powershell#2135.

  • VSCode Version: 1.37.0
  • OS Version: Windows_NT x64 10.0.17763

Steps to Reproduce:

See GIF.

Does this issue occur when all extensions are disabled?: No -- need an extension for a CodeLens

We've been looking into this on the PowerShell extension side, but see this message in the logs:

    {
      "jsonrpc": "2.0",
      "id": 8,
      "method": "textDocument/codeAction",
      "params": {
        "textDocument": {
          "uri": "file:///c%3A/Users/Keith/.vscode/extensions/ms-vscode.powershell-2019.5.0/examples/2135_Repro.Tests.ps1"
        },
        "range": {
          "start": {
            "line": 2,
            "character": 8
          },
          "end": {
            "line": 2,
            "character": 1.7976931348623157E+308
          }
        },
        "context": {
          "diagnostics": []
        }
      }
    }

This value for range.end.character causes the extension to crash, but there's not much sane we can do with it. It's equal to Number.MAX_VALUE in JS.

@rebornix rebornix assigned mjbvz and unassigned rebornix Sep 4, 2019
@mjbvz
Copy link
Collaborator

mjbvz commented Sep 9, 2019

Can you please write out the steps to reproduce this (instead of a gif)

@mjbvz mjbvz added the info-needed Issue requires more information from poster label Sep 9, 2019
@rjmholt
Copy link
Author

rjmholt commented Sep 10, 2019

  • Install the powershell-preview extension to provide this problem matcher
  • Ensure PowerShell is installed
  • Install Pester (Install-Module Pester in PowerShell)
  • Create foo.ps1:
Describe "Repro" {
    It "Test" -Test {
        3 % 2 | Should -BeExactly 0
    }
}
  • Add the following to the tasks.json:
{
    "label": "Test",
    "type": "shell",
    "command": "Invoke-Pester -PesterOption @{IncludeVSCodeMarker=$true}",
    "group": {
        "kind": "test",
        "isDefault": true
    },
    "problemMatcher": [
        "$pester"
    ]
},
  • In the Command Palette, run Tasks: Run Test Task
  • When a red squiggle appears under the test, click on QuickFix (some users hit this with only a hover over the red squiggle)

Pester writes an output to stdout and VSCode parses this. This output provides a line, but not a column. VSCode highlights this line, but then sends Number.MAX_VALUE as the column end character to our language server, which is too big to be deserialised as a fixed-width integer (e.g. a long). A value like Number.MAX_SAFE_INTEGER would work just as well here (that many characters represents ~8000 TiB) but wouldn't require arbitrary precision integers (which require allocation or special handling at the deserialisation layer) in languages where numbers are not all floating point.

I appreciate that the LSP spec specifies this value as a number not an integer, but I feel that a floating point number of characters (which Number.MAX_VALUE is not just in .NET and Java but also JS, since Number.MAX_VALUE === Number.MAX_VALUE + 1) isn't meaningful or useful to send to servers.

@mjbvz mjbvz removed the info-needed Issue requires more information from poster label Sep 20, 2019
@mjbvz mjbvz added this to the October 2019 milestone Sep 20, 2019
@mjbvz mjbvz added the bug Issue identified by VS Code Team member as probable bug label Sep 20, 2019
@jrieken jrieken added the editor-code-actions Editor inplace actions (Ctrl + .) label Oct 16, 2019
@mjbvz
Copy link
Collaborator

mjbvz commented Oct 25, 2019

Just got around to looking into this but I can't reproduce the diagnostic showing up:

Screen Shot 2019-10-25 at 4 44 26 PM

@rjmholt Are there any extra steps required here? I'm not a heavy powershell/windows user so its possible my machine is not configured properly

@mjbvz mjbvz added the info-needed Issue requires more information from poster label Oct 25, 2019
@mjbvz
Copy link
Collaborator

mjbvz commented Oct 31, 2019

@rjmholt Any details on reproducing this?

@mamidenn
Copy link

mamidenn commented Oct 31, 2019

@mjbvz Please try renaming foo.ps1 to foo.Tests.ps1 and reproduce as described above.

@rjmholt
Copy link
Author

rjmholt commented Oct 31, 2019

Any details on reproducing this?

Sorry, lost the original notification!

Please try renaming foo.ps1 to foo.Tests.ps1 and reproduce as described above.

Ah, yes, please give that a try.

The issue itself lies in how the problem matcher handles not being given a column, so if that doesn't work I might be able to craft a more minimal scenario (that will just take time).

@kieferrm kieferrm modified the milestones: October 2019, November 2019 Oct 31, 2019
@mjbvz
Copy link
Collaborator

mjbvz commented Nov 14, 2019

Now I can run the test but I don't see any problem squiggles in foo.Test.ps1:

Screen Shot 2019-11-13 at 10 48 09 PM

Instead, it is highlighting the error in Should.ps1:

Screen Shot 2019-11-13 at 10 48 41 PM

@mamidenn
Copy link

It appears you are running an old version of Pester (3.4.0 - latest is 4.9.0), which does not yet have -BeExactly. You could try updating Pester using Update-Module Pester.

Or you could try adapting foo.Tests.ps1 to be

Describe "Repro" {
    It "Test" -Test {
        3 % 2 | Should -Be 0
    }
}

I was able to reproduce the issue using Pester 3.4.0 and the above script.

Thanks for keeping it up 😃!

@mjbvz
Copy link
Collaborator

mjbvz commented Nov 20, 2019

Thanks. I can finally repo this.

The root cause seems to be that the marker itself (the problem) has an invalid range. The code action itself just surfaces this range. I confirmed that the marker service is storing a marker that has an endColumn value of 1.7976931348623157e+308

This comes from the problemMatcher:

return { startLineNumber: startLine, startCharacter: 1, endLineNumber: startLine, endCharacter: Number.MAX_VALUE };

So if the problem matcher returned the end column, this wouldn't happen.

I think I can also fix the code action provider to clip the range before passing it on however

@mjbvz mjbvz removed the info-needed Issue requires more information from poster label Nov 20, 2019
@mjbvz mjbvz closed this as completed in 4934a6f Nov 20, 2019
@mjbvz
Copy link
Collaborator

mjbvz commented Nov 20, 2019

Made a fix but the Powershell extension still seems to crash when requesting the code actions. I confirmed that we are passing a more sane range now however

@rjmholt
Copy link
Author

rjmholt commented Nov 20, 2019

Not surprised we still crash; we're quite sensitive to line offsets.

But if the crash is happening after deserialisation, we're in business.

@aeschli
Copy link
Contributor

aeschli commented Dec 5, 2019

@rjmholt If you can verify the fix works for you that would be great. Set the verified label if it does, or reopen.

@jrieken jrieken added the verification-needed Verification of issue is requested label Dec 6, 2019
@jrieken
Copy link
Member

jrieken commented Dec 6, 2019

@mjbvz Are there verification steps?

@JacksonKearl JacksonKearl added verified Verification succeeded and removed verified Verification succeeded labels Dec 6, 2019
@mjbvz mjbvz added this to the February 2020 milestone Jan 30, 2020
@mjbvz mjbvz modified the milestones: February 2020, March 2020 Feb 27, 2020
@mjbvz mjbvz modified the milestones: March 2020, April 2020 Mar 31, 2020
@mjbvz mjbvz modified the milestones: April 2020, May 2020 Apr 30, 2020
@mjbvz mjbvz modified the milestones: May 2020, June 2020 Jun 1, 2020
@mjbvz
Copy link
Collaborator

mjbvz commented Jun 27, 2020

After snowplowing for like six iterations iterations, it's time to admit I'm not going to get around to this issue anytime soon.

Marking as help wanted. @TylerLeonhardt It'd probably be best if someone from the Powershell extension could investigate this. It may be a bug on the VS Code side but so far I've only seen reports of it effecting Powershell

@mjbvz mjbvz modified the milestones: June 2020, Backlog Jun 27, 2020
@mjbvz mjbvz added help wanted Issues identified as good community contribution opportunities and removed verification-found Issue verification failed verification-needed Verification of issue is requested labels Jun 27, 2020
@TylerLeonhardt
Copy link
Member

I mean this affects any language extension that uses:

https://github.com/OmniSharp/csharp-language-server-protocol

Which includes the ARM extension, the Razor, extension, and more.

Because that library tries to serialize the column into the int, but vscode defaults to some massive number outside of that range when it can't find a column.

If vscode defaulted to something within the range of int, this would resolve the issue.

@mjbvz
Copy link
Collaborator

mjbvz commented Jun 27, 2020

It would be helpful to have someone from any of effected extensions investigate this.

I don't deal with any of those languages/extensions regularly, so just setting up a repo for this bug is a big time sink. Someone with more knowledge of how these extensions are implemented should be able to pinpoint what is going wrong much more quickly

@TylerLeonhardt
Copy link
Member

Without knowing any of the code, I wonder if it has something to do at the serialization layer, when column is set to undefined

@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Jun 27, 2020

@mjbvz the problem is this line:

return { startLineNumber: startLine, startCharacter: 1, endLineNumber: startLine, endCharacter: Number.MAX_VALUE };

It uses Number.MAX_VALUE which is 1.7976931348623157e+308 which isn't really safe to use since it just represents the largest value that javascript can "handle". Number.MAX_SAFE_INTEGER might be a better option because it represents 2^53 - 1 which is a standard 64bit integer... but that could cause problems with language servers that use 32bit integers (like ours).

The best value for the default for this would be: 2^31-1, or 2,147,483,647 which is the largest standard 32bit integer.

I verified that setting that line to 2147483647 no longer throws an exception in the PowerShell language server. If you wanna verify it yourself, @rjmholt's steps are all you need to do: #80288 (comment)

@mjbvz how would you like to proceed here? Surely that value is large enough to be the default?

@mjbvz
Copy link
Collaborator

mjbvz commented Jun 29, 2020

Thanks for the investigation!

Adding @alexr00 for the problem matcher. Using a large number like 2^31-1 sounds reasonable

@TylerLeonhardt
Copy link
Member

It might be worth spot checking some of the other usages of Number.MAX_INT (I saw a few in the code) in favor of an max 32bit int constant used everywhere.

@alexr00
Copy link
Member

alexr00 commented Jun 30, 2020

2147483647 seems fine for the end character position. I'll make the change for the July iteration.

@alexr00 alexr00 modified the milestones: Backlog, July 2020 Jun 30, 2020
@alexr00 alexr00 closed this as completed in 15c5d14 Jul 8, 2020
gjsjohnmurray pushed a commit to gjsjohnmurray/vscode that referenced this issue Jul 8, 2020
@roblourens roblourens added the verified Verification succeeded label Aug 6, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Aug 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug editor-code-actions Editor inplace actions (Ctrl + .) help wanted Issues identified as good community contribution opportunities insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

12 participants