Skip to content

🐛 fix: correct symbol handling, index_by test, and inlay hints logic#1421

Merged
harehare merged 2 commits intomainfrom
fix-symbol-expr-indexby-inlay
Mar 11, 2026
Merged

🐛 fix: correct symbol handling, index_by test, and inlay hints logic#1421
harehare merged 2 commits intomainfrom
fix-symbol-expr-indexby-inlay

Conversation

@harehare
Copy link
Copy Markdown
Owner

  • Fix symbol handling in Hir by using add_expr for arguments
  • Update index_by test to use correct dict syntax
  • Fix inlay hints logic to check for concrete types

- Fix symbol handling in Hir by using add_expr for arguments
- Update index_by test to use correct dict syntax
- Fix inlay hints logic to check for concrete types
Copilot AI review requested due to automatic review settings March 11, 2026 12:20
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a few correctness issues across the HIR builder, builtin tests, and WASM inlay-hints generation to better align with actual language semantics and type-checker behavior.

Changes:

  • Update HIR foreach handling to register the iterable expression via add_expr rather than a single Ref symbol.
  • Fix the index_by builtin test to use dict literal syntax.
  • Update WASM inlay hints to only emit hints when the inferred type is fully concrete.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
crates/mq-wasm/src/script.rs Tightens inlay-hints emission by requiring concrete inferred types.
crates/mq-lang/builtin_tests.mq Adjusts index_by test input to valid dict literal syntax.
crates/mq-hir/src/hir.rs Changes foreach iterable symbol handling to use recursive expression registration (add_expr).

doc: node.comments(),
parent: Some(symbol_id),
});
self.add_expr(arg, source_id, scope_id, Some(symbol_id));
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

In foreach, the iterable expression (arg) is evaluated in the outer environment before the loop binding is introduced (see mq-lang/src/eval.rs eval_foreach: it evaluates values before creating the new loop env). Adding the iterable expression into the new loop scope can cause name resolution bugs for cases like foreach(x, x): ... where the iterable x should refer to the outer binding, not the loop variable declared on the same line. Consider building symbols for the iterable expression in the parent scope (keep the original scope_id before creating the loop scope), and only use the loop scope for the loop variable and body.

Copilot uses AI. Check for mistakes.
Comment on lines +517 to 518
if !type_scheme.ty.is_concrete() {
return None;
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

Type::is_concrete() currently allocates and collects all type variables via free_vars(); calling this for every symbol when generating inlay hints can add noticeable overhead for large files (especially in WASM). If you only need a boolean “contains any type var”, consider adding a short-circuiting helper (e.g., has_type_vars() / contains_var()) that walks the type tree without allocating, and use that here instead of is_concrete().

Copilot uses AI. Check for mistakes.
@harehare harehare merged commit 0fe5933 into main Mar 11, 2026
4 checks passed
@harehare harehare deleted the fix-symbol-expr-indexby-inlay branch March 11, 2026 12:43
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Mar 11, 2026

Merging this PR will not alter performance

✅ 29 untouched benchmarks


Comparing fix-symbol-expr-indexby-inlay (82a30f7) with main (795b7e1)1

Open in CodSpeed

Footnotes

  1. No successful run was found on main (82a30f7) during the generation of this report, so 795b7e1 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

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.

2 participants