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 fallback logic in analysis.getPositionContext #1754

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

llogick
Copy link
Member

@llogick llogick commented Feb 9, 2024

Most lines are indented, ie line[0] == ' '(a space).

Tokenizing line as the sole buffer returns location offsets relative/within to that line, but are expected to be indexes within handle.tree.source .

Copy link

codecov bot commented Feb 9, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (6478f1a) 76.58% compared to head (b4d49a9) 76.59%.

Files Patch % Lines
src/analysis.zig 90.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1754   +/-   ##
=======================================
  Coverage   76.58%   76.59%           
=======================================
  Files          35       35           
  Lines       10088    10090    +2     
=======================================
+ Hits         7726     7728    +2     
  Misses       2362     2362           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Techatrix Techatrix left a comment

Choose a reason for hiding this comment

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

This looks like something that could be covered by a test case in position_context.zig.

@llogick llogick force-pushed the nullptrdevs/fix-fallback-logic-in-getPositionContext branch from 63a54a8 to 161e51f Compare February 11, 2024 07:10
@llogick
Copy link
Member Author

llogick commented Feb 11, 2024

Having done this makes me realize having that logic is very dicey and

handle.tree.tokens.items(.start)[el_dot_context.identifier_token_index],

should just use tok_loc.end or
switch (try Analyser.getPositionContext(arena, handle.tree.source, source_index, false)) {

look_ahead = true here (haven't tested for side effects).

@llogick llogick force-pushed the nullptrdevs/fix-fallback-logic-in-getPositionContext branch from 161e51f to b4d49a9 Compare February 11, 2024 18:31
@llogick
Copy link
Member Author

llogick commented Apr 13, 2024

This is a low priority PR, the test changes are worth having, but in my opinion the fallback logic shouldn't exist (and obviously nothing depends on it currently).

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

Successfully merging this pull request may close these issues.

None yet

2 participants