Add lsp range info for file load errors #7335
Conversation
faldor20
force-pushed
the
add-lsp-range-info-for-file-load-errors-
branch
2 times, most recently
from
6af627a to
82f6d29
Compare
faldor20
force-pushed
the
add-lsp-range-info-for-file-load-errors-
branch
from
d2b3733 to
e698953
Compare
|
I don't really understand that fuzzing test, or how I could have broken it. |
|
@faldor20 seems like we'll be disabling the fuzzing step for now: https://roc.zulipchat.com/#narrow/channel/395097-compiler-development/topic/Weird.20fuzzing.20bug.20of.20the.20day/near/488547602 |
| .get_region() | ||
| .unwrap_or(Region::new( | ||
| roc_region::all::Position::new(0), | ||
| roc_region::all::Position::new(10000000), |
There was a problem hiding this comment.
This will presumably make the whole screen red if we don't have a region. Maybe just the first char is a better default? If not, then maybe we should use usize::MAX instead of "some really big number".
There was a problem hiding this comment.
USize max is a bad plan. Lsp and json goes to javascript which, we must remember stores numbers as 64bit float and that would overflow on a usezie from 64bit systems.
This is stupid large and should be fine in all practical applications, but I guess I could use f64max.
We do this because the main cases where we don't have a target it's because the whole document is cooked in some way.
As Anton mentioned in zulip, it's often hard to find these errors if they are just on line 1 or something. I agree. If the error is a blocker for even parsing which precludes any other errors, why not show the whole doc as an erorr
There was a problem hiding this comment.
But I guess we don't care right, because it won't actually overflow it'll just be imprecise.. I still don't entirely trust it to handle super large numbers maybe I'm just too conservative 😅
I guess I worry the imprecision will do funky things.
There was a problem hiding this comment.
My arbitrary number should be about 10million chars is about 100k lines of code which is as much as anyone should ever need... 10k might be a little too small.
|
There's nothing blocking merging this IMO, but I didn't approve since there's a CI check failing. Feel free to ping me when that's fixed and I can approve without worry of the approval being dismissed by code changes. |
|
Thanks so much for this change! I started working on this at one point because it's really annoying when this happens and I end up needing to run |
|
Can you actually sign all of these commits @faldor20 ? https://github.com/roc-lang/roc/blob/main/CONTRIBUTING.md#commit-signing |
compiling, but still missing positions from expression errors
faldor20
force-pushed
the
add-lsp-range-info-for-file-load-errors-
branch
from
1739859 to
c731c5d
Compare
|
there you go @smores56 I think it lost my signatures because I got github to rebase onto master |
This now makes the language server use the correct range when encountering a file load error.
So now parse errors won't all appear at the very top!