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

fix: Correct off-by-one errors in lexer spans #2393

Merged
merged 3 commits into from
Aug 24, 2023
Merged

Conversation

TomAFrench
Copy link
Member

@TomAFrench TomAFrench commented Aug 21, 2023

Description

Problem*

Closes #1682
Resolves #1683

Summary*

This PR addresses the off-by-one errors in how we deal with spans in a way which makes sense to me.

For example in parse_block_comment we're creating a span which goes from the start of the comment to the very last character in the file. We should then have an inclusive span here. Similarly when we have a && we were previously creating a span of size 1 which doesn't really make sense for when we're capturing two characters.

Based on this, I noticed that from_position was creating exclusive end spans whereas we're calling it in the same way where logically we should have inclusive spans. I then switched this and the red squigglies are all appearing where I expect them to be.

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@TomAFrench TomAFrench requested review from phated and jfecher August 21, 2023 23:03
@TomAFrench TomAFrench changed the title fix: correct off-by-one errors in lexer spans fix: Correct off-by-one errors in lexer spans Aug 21, 2023
Copy link
Contributor

@phated phated left a comment

Choose a reason for hiding this comment

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

I'll give this a try in a bit tomorrow (I didn't realize how late it was)

@TomAFrench TomAFrench self-assigned this Aug 22, 2023
jfecher
jfecher previously approved these changes Aug 22, 2023
Copy link
Contributor

@phated phated left a comment

Choose a reason for hiding this comment

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

Screenshot 2023-08-22 at 3 19 30 PM

I found more span problems even with these changes included. I'll push up my changes shortly

@phated
Copy link
Contributor

phated commented Aug 22, 2023

Two more issues with strings:
Screenshot 2023-08-22 at 3 25 19 PM

@phated
Copy link
Contributor

phated commented Aug 22, 2023

Pushed my new changes that fix all of the above:
Screenshot 2023-08-22 at 3 26 11 PM

@phated phated requested a review from jfecher August 22, 2023 22:29
phated
phated previously approved these changes Aug 22, 2023
Copy link
Contributor

@phated phated left a comment

Choose a reason for hiding this comment

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

Approving, but I made changes so let's get other feedback

@phated phated requested a review from kevaundray August 22, 2023 22:29
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

LGTM. If any more off by one spans are found we can fix them as part of another PR

@kevaundray kevaundray added this pull request to the merge queue Aug 24, 2023
Merged via the queue into master with commit bbda9b0 Aug 24, 2023
@kevaundray kevaundray deleted the fix-lexer-spans branch August 24, 2023 15:12
TomAFrench added a commit that referenced this pull request Aug 24, 2023
* master:
  chore(ci): Fix version of `cross` to 0.2.5 (#2426)
  fix: Correct off-by-one errors in lexer spans (#2393)
TomAFrench added a commit that referenced this pull request Aug 24, 2023
* master:
  feat: create equivalence relationships for intermediate witnesses from multiplication (#2414)
  chore(ci): Fix version of `cross` to 0.2.5 (#2426)
  fix: Correct off-by-one errors in lexer spans (#2393)
TomAFrench added a commit that referenced this pull request Aug 24, 2023
* master:
  chore: improve error message for InternalError (#2429)
  chore: Add stdlib to every crate as it is added to graph (#2392)
  feat: create equivalence relationships for intermediate witnesses from multiplication (#2414)
  chore(ci): Fix version of `cross` to 0.2.5 (#2426)
  fix: Correct off-by-one errors in lexer spans (#2393)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Codespans are off-by-one
4 participants