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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed out of bounds in line mapping #1707

Merged
merged 8 commits into from Feb 13, 2020
Merged

fixed out of bounds in line mapping #1707

merged 8 commits into from Feb 13, 2020

Conversation

@filipw
Copy link
Member

filipw commented Feb 10, 2020

Fixes OmniSharp/omnisharp-vscode#3485
Also added more comprehensive tests.

Here is the summary of the logic:

  • correct mapping within a file - then we return the mapped line and mapped text
  • incorrect mapping within a file - then we return the mapped line (even though it's incorrect) and the original (unmapped) text since there is no way to fetch the mapped text as it's out of bounds
  • correct mapping into a file that exists in a workspace - then we return the mapped line and mapped text from that file
  • incorrect mapping into a file that exists in a workspace - then we return the mapped line and the original (unmapped) text since there is no way to fetch the mapped text as it's out of bounds
  • mapping into a file that does not exist in a workspace - then we return the mapped line and the original (unmapped) text since there is no way to fetch the mapped text as we don't have access to it (Razor case). The editor still ignores it anyway and shows the correct file in this case.

cc @NTaylorMullen as this bug affected Razor 馃檲

filipw added 5 commits Feb 8, 2020
@filipw filipw requested review from JoeRobich, david-driscoll and mholo65 Feb 10, 2020
Copy link
Collaborator

NTaylorMullen left a comment

鈾ワ笍

@filipw

This comment has been minimized.

Copy link
Member Author

filipw commented Feb 12, 2020

@JoeRobich @david-driscoll any objections? can this be merged?

Copy link
Collaborator

JoeRobich left a comment

I am fine with this change. Just not an area I know much about. =)

if (hasMappedPath)
{
SourceText source = null;
if (documents != null && documents.Any() && documents.FirstOrDefault(d => d.TryGetText(out source)) != null)

This comment has been minimized.

Copy link
@JoeRobich

JoeRobich Feb 12, 2020

Collaborator

I'm not as familiar with line mappings. Would it be more correct to loop through the documents where you can get text and see if the mapping applies?

This comment has been minimized.

Copy link
@JoeRobich

JoeRobich Feb 12, 2020

Collaborator

documents.Any() isn't necessary now that you are using FirstOrDefault

This comment has been minimized.

Copy link
@filipw

filipw Feb 12, 2020

Author Member

I think this can only ever return more than 1 in case of multi targeting so the document text would be identical

@david-driscoll

This comment has been minimized.

Copy link
Member

david-driscoll commented Feb 13, 2020

:shipit:

@filipw filipw merged commit d443479 into master Feb 13, 2020
6 checks passed
6 checks passed
OmniSharp.omnisharp-roslyn Build #1.34.12-PullRequest1707.16 succeeded
Details
OmniSharp.omnisharp-roslyn (GitVersion) GitVersion succeeded
Details
OmniSharp.omnisharp-roslyn (Linux) Linux succeeded
Details
OmniSharp.omnisharp-roslyn (Release) Release succeeded
Details
OmniSharp.omnisharp-roslyn (Windows) Windows succeeded
Details
OmniSharp.omnisharp-roslyn (macOS) macOS succeeded
Details
@filipw filipw deleted the bugfix/locationextensions branch Feb 13, 2020
filipw added a commit that referenced this pull request Feb 14, 2020
mholo65 added a commit that referenced this pull request Feb 15, 2020
PRs #1707 and #1701 in changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can鈥檛 perform that action at this time.