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

[DSLX] Factor out diagnostic routines, make test, add one diagnostic routine. #1827

Merged

Conversation

cdleary
Copy link
Collaborator

@cdleary cdleary commented Jan 4, 2025

Now we give a more helpful explanation if you put a trailing semicolon in one block within an if/else ladder that gives an indication that users should see if that was intended. I observed it was common for folks to type an accidental trailing semi and then try to decipher why they were getting the error particularly if the consequent/alternate blocks were large.

  • Most of this change by volume is refactoring things into the diagnostics/ dir
  • But it also adds xls/dslx/diagnostics/maybe_explain_error_test.cc
  • It exposes two internal routines (in an internal namespace) to enable that test -- previously all diagnostic tests had to be done at the language level
  • Conditional::GatherBlocks is added to collect all the blocks participating in an if/else ladder
  • AstNode::SetEncosing() / GetEnclosing() is added to supplement the parent() relation -- because conditional nodes nest we want a direct (and easy) link to the "primordial" conditional in the ladder, and if you traversed parent() links you would have trouble telling when to stop (e.g. consider two nested conditional ladders) -- parent() relation is already completely defined as "tell me the node who claims me via GetChildren()" so we needed something new or a complicated traversal criterion

@copybara-service copybara-service bot merged commit f23cf22 into google:main Jan 6, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants