fix: add better errors for missing commas in arrays and objects #5210

jtran · 2025-01-31T23:52:42Z

Authored by @TomPridham. Replaces/closes #5079.

closes #5055.

Adds better error messaging for missed commas in objects and arrays and adds the same missing closing bracket message for objects

qa-wolf · 2025-01-31T23:52:45Z

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

vercel · 2025-01-31T23:52:47Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
modeling-app	✅ Ready (Inspect)	Visit Preview	Feb 11, 2025 6:13pm

TomPridham · 2025-02-01T23:49:08Z

I pushed an update that made the wording clearer in the case of random characters being in there like the [}] example that failed. I also updated the test and added some new ones for that case. I think the yarn unit tests should pass now, but I was having trouble running them locally

codecov · 2025-02-02T05:08:52Z

Codecov Report

Attention: Patch coverage is 98.57143% with 2 lines in your changes missing coverage. Please review.

Project coverage is 85.85%. Comparing base (d0c8311) to head (2b0944f).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/wasm-lib/kcl/src/parsing/parser.rs	98.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5210      +/-   ##
==========================================
+ Coverage   85.80%   85.85%   +0.05%     
==========================================
  Files          94       94              
  Lines       33554    33676     +122     
==========================================
+ Hits        28792    28914     +122     
  Misses       4762     4762

Flag	Coverage Δ
wasm-lib	`85.85% <98.57%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

nrc

These are much nicer messages, thanks! I think we need to get change the approach of scanning for the closing delimiter though (or do that for all delimiters in a pre-parsing pass over the tokens, which might be useful for better error messages elsewhere, but if it's a lot of work I would avoid it since long-term we probably need to rewrite the parser) to avoid performance issues and over-matching

nrc · 2025-02-10T16:10:05Z

src/lang/abstractSyntaxTree.test.ts

+    expect(error.message).toBe(
+      'Unexpected character encountered. You might be missing a comma in between elements.'
+    )
+    expect(error.sourceRange).toEqual([29, 31, 0])


I'd prefer we didn't test error messages from type script, can we just remove this check (or the whole file)?

Remove unneeded AST test in TypeScript #5343

nrc · 2025-02-10T16:18:40Z

src/wasm-lib/kcl/src/parsing/parser.rs

-        })?
-        .end;
+
+    let maybe_closing_bracket: PResult<TokenSlice<'_>> = peek(take_until(0.., "]")).parse_next(i);


This seems a little eager to find a closing bracket - it can scan to the end of the source file(unless I misunderstand what Winnow would do) so could get a lot of false positives (as well as being bad for performance). At the least, we shouldn't do this check until we know we are recovering from an error (and even then it feels bad since we might call this function to test if this is an array). But I think for correctness the forward search should be terminated by anything which can't occur before a ] in correct code.

Yeah, you're right. I can definitely move the check into the if statement on 761. How would you suggest checking for incorrect literals before the ]? If I'm thinking about it correctly, we would need to track the literals as we progressed through the tokens.
Like, a parenthesis could be invalid, but only if it's not balanced. Same with a comma. ,, is definitely wrong, but multiple commas aren't inherently incorrect

I moved the closing bracket checks into the error branch. Let me know about how best to go about bailing out earlier and I can add that in as well

Yeah, it's not easy! I don't know how best to integrate it with Winnow. Typically for this kind of recovery you come up with a set of tokens which cannot appear within the sequence, the easiest place to start is keywords. For matching delimiters you need to use your own stack.

Okay, I got that working as requested I think. I didn't do anything too fancy for things like braces, brackets, and commas. Let me know if I should adjust or add anything in. I had some other comments/questions I'll leave on this PR once it's up to date with my branch

nrc · 2025-02-10T16:21:42Z

src/wasm-lib/kcl/src/parsing/parser.rs

+    ignore_trailing_comma(i);
+    ignore_whitespace(i);
+
+    let maybe_closing_brace: PResult<TokenSlice<'_>> = peek(take_until(0.., "}")).parse_next(i);


nrc · 2025-02-10T16:22:32Z

src/wasm-lib/kcl/src/parsing/parser.rs

@@ -3769,7 +3820,7 @@ height = [obj["a"] - 1, 0]"#;
    #[test]
    fn test_parse_member_expression_binary_expression_in_array_number_second_missing_space() {
        let code = r#"obj = { a: 1, b: 2 }
-height = [obj["a"] -1, 0]"#;
+    height = [obj["a"] -1, 0]"#;


This implies there's indentation in the KCL code, which is probably not intended

jtran requested a review from adamchalmers January 31, 2025 23:52

vercel bot deployed to Preview February 1, 2025 00:03 View deployment

jtran force-pushed the jtran/kc-5055/missing-comma-message branch from 45a913f to 620ea21 Compare February 2, 2025 04:40

vercel bot deployed to Preview February 2, 2025 04:51 View deployment

TomPridham added 3 commits February 4, 2025 20:00

fix: add better errors for missing commas in arrays and objects

e706208

chore: add object prop shorthand missing comma test

3d8089a

fix: wording on unexpected character in arrays and objects

4ddf3da

jtran force-pushed the jtran/kc-5055/missing-comma-message branch from 620ea21 to 4ddf3da Compare February 5, 2025 01:00

vercel bot deployed to Preview February 5, 2025 01:12 View deployment

nrc reviewed Feb 10, 2025

View reviewed changes

jtran and others added 2 commits February 11, 2025 12:58

Merge branch 'main' into jtran/kc-5055/missing-comma-message

ff891e3

fix: don't eagerly evaluate whether there is a closing brace/bracket

2b0944f

jtran mentioned this pull request Feb 11, 2025

Remove unneeded AST test in TypeScript #5343

Merged

vercel bot deployed to Preview February 11, 2025 18:13 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add better errors for missing commas in arrays and objects #5210

fix: add better errors for missing commas in arrays and objects #5210

jtran commented Jan 31, 2025

qa-wolf bot commented Jan 31, 2025

vercel bot commented Jan 31, 2025 •

edited

Loading

TomPridham commented Feb 1, 2025

codecov bot commented Feb 2, 2025 •

edited

Loading

nrc left a comment

nrc Feb 10, 2025

jtran Feb 11, 2025 •

edited

Loading

nrc Feb 10, 2025

TomPridham Feb 11, 2025

TomPridham Feb 11, 2025

nrc Feb 11, 2025

TomPridham Feb 13, 2025

nrc Feb 10, 2025

nrc Feb 10, 2025

fix: add better errors for missing commas in arrays and objects #5210

Are you sure you want to change the base?

fix: add better errors for missing commas in arrays and objects #5210

Conversation

jtran commented Jan 31, 2025

qa-wolf bot commented Jan 31, 2025

vercel bot commented Jan 31, 2025 • edited Loading

TomPridham commented Feb 1, 2025

codecov bot commented Feb 2, 2025 • edited Loading

Codecov Report

nrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtran Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Jan 31, 2025 •

edited

Loading

codecov bot commented Feb 2, 2025 •

edited

Loading

jtran Feb 11, 2025 •

edited

Loading