Skip to content

Add warning for stray standalone is statements#431

Open
lwaern-intel wants to merge 7 commits intointel:mainfrom
lwaern-intel:lw/stray-is-check
Open

Add warning for stray standalone is statements#431
lwaern-intel wants to merge 7 commits intointel:mainfrom
lwaern-intel:lw/stray-is-check

Conversation

@lwaern-intel
Copy link
Copy Markdown
Contributor

@lwaern-intel lwaern-intel commented Mar 30, 2026 •
edited
Loading

Depends on #430; only the latter two commits shown in this PR are relevant for review.

lwaern-intel
lwaern-intel commented Mar 30, 2026
View reviewed changes
py/dml/messages.py Outdated
or
```
field f @ [31:0];
is read_only;
Copy link
Copy Markdown
Contributor Author

@lwaern-intel lwaern-intel Mar 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

as you could probably guess it's this second part that first led me down the lexpos rabbit hole. though when I realized it was also why all error messages have been screwed up since forever, it kinda got personal

@syssimics
Copy link
Copy Markdown
Contributor

syssimics commented Mar 30, 2026

DML Verification : 6: ❌ failure

@syssimics
Copy link
Copy Markdown
Contributor

syssimics commented Mar 30, 2026

DML Verification : 7: ❌ failure

@lwaern-intel lwaern-intel force-pushed the lw/stray-is-check branch 2 times, most recently from f9566ba to 16131c6 Compare March 30, 2026 23:16
@lwaern-intel lwaern-intel mentioned this pull request Mar 30, 2026
Open
@syssimics
Copy link
Copy Markdown
Contributor

syssimics commented Mar 30, 2026

PR Verification: ❌ failure

@syssimics
Copy link
Copy Markdown
Contributor

syssimics commented Mar 31, 2026

PR Verification: ✅ success

mandolaerik
mandolaerik reviewed Mar 31, 2026
View reviewed changes
mandolaerik
mandolaerik reviewed Mar 31, 2026
View reviewed changes
@mandolaerik
Copy link
Copy Markdown
Contributor

mandolaerik commented Mar 31, 2026 •
edited
Loading

Dealing with lexical details from the parser has a somewhat unhygienic smell about it; perhaps a different approach might be to do an isolated check on lexer level, essentially the equivalent of re.search("field [^\n]*; *is"). Which admittedly also has a bad smell to it, but a different one.

I think the emptyprod fixup looks good, and can be merged separately first if you want to. oh you did that already

@syssimics syssimics requested a review from mandolaerik March 31, 2026 16:54
@syssimics
Copy link
Copy Markdown
Contributor

syssimics commented Mar 31, 2026

PR Verification: ✅ success

@syssimics
Copy link
Copy Markdown
Contributor

syssimics commented Mar 31, 2026

PR Verification: ✅ success

@lwaern-intel lwaern-intel force-pushed the lw/stray-is-check branch 2 times, most recently from 3659e2f to 67dc202 Compare March 31, 2026 18:15
@lwaern-intel
Copy link
Copy Markdown
Contributor Author

lwaern-intel commented Mar 31, 2026 •
edited
Loading

@mandolaerik

Dealing with lexical details from the parser has a somewhat unhygienic smell about it; perhaps a different approach might be to do an isolated check on lexer level, essentially the equivalent of re.search("field [^\n]*; *is"). Which admittedly also has a bad smell to it, but a different one.

The rough_end_site bit makes me inclined to agree with you that this solution is dirty, but I'm not convinced that doing it on the lexer level would be any better or easier. In particular, I'm dubious towards a regex because whatever heuristic we use for the warning has to be very context-sensitive. Even with the simplest pattern -- everything being on the same line -- we have to be careful it could not possible match within a comment, or a method, etc. The second pattern -- an is being indented underneath -- seems nightmarish to get right.

Working on the ast level on the other hand makes is trivial to establish context and let us understand perfectly what situations the warning is emitted. That's kind of a killer benefit and seems hard to beat.

In addition I'll note this is not completely without precedence, as port_dml relies on the line and column number reported by the porting messages in order to perform its transformations.

@lwaern-intel lwaern-intel requested a review from Copilot March 31, 2026 18:43
Copilot AI reviewed Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown

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 improves source-location accuracy in the DML parser (especially around empty productions) and adds a new warning to catch likely-mistaken standalone is statements that appear to be intended to modify the immediately preceding object declaration.

Changes:

  • Track the latest lexed token during parsing and fix up lexpos for empty productions to improve reported line/column locations.
  • Add warning WSTRAYIS plus parser-side detection to flag suspicious standalone is statements.
  • Add regression tests (DML error test + Python unit test) and document the behavior in release notes.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
test/1.4/errors/T_WSTRAYIS.dml Adds an errors/warnings test case covering the new WSTRAYIS warning.
RELEASENOTES.md Documents the lexpos fix impact and the new stray-is warning.
py/dml/toplevel.py Passes a custom tokenfunc into PLY parsing to enable latest-token tracking.
py/dml/messages.py Introduces the WSTRAYIS warning definition and message text.
py/dml/logging.py Adds colno to Site and adjusts FileInfo state serialization and column calculation.
py/dml/dmlparse.py Implements empty-production lexpos fixups and adds the stray-is detection pass.
py/dml/dmlparse_test.py Adds unit tests to enforce empty-production fixups and validate site/column behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@lwaern-intel lwaern-intel requested a review from Copilot March 31, 2026 19:04
Copilot AI reviewed Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown

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

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

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@lwaern-intel lwaern-intel requested a review from Copilot March 31, 2026 19:26
Copilot AI reviewed Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown

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

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@lwaern-intel lwaern-intel requested a review from Copilot March 31, 2026 20:38
Copilot AI reviewed Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown

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

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

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@lwaern-intel lwaern-intel requested a review from Copilot March 31, 2026 20:59
Copilot AI reviewed Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown

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

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@syssimics
Copy link
Copy Markdown
Contributor

syssimics commented Mar 31, 2026

PR Verification: ✅ success

lwaern-intel and others added 6 commits April 5, 2026 12:45
@lwaern-intel
Fix up lexpos for symbols from empty productions -- SIMICS-23466
f29c5fb
@mandolaerik @lwaern-intel
Add make dependency
2c70f6d 
Otherwise the parsetab may be built implicitly when dmlparse_test is run
@mandolaerik @lwaern-intel
Tighten error condition
c11ce4f 
Fixup function should not be called on nonempty rules

Also rewrite as unittest
@lwaern-intel
Tidy away vestigial hack
f8993c3
@lwaern-intel
Add warning for stray standalone is statements
4f84a27
@lwaern-intel
Simpler and more conservative stray_is_check
b9e5e66 
Previous iteration went with the stance of "as long as multiple declarations
are on the same line, assume SOMETHING is messed up, no matter what the
declarations are."
This is not necessarily a wrong stance to take, trouble is that in those cases
emitting `WSTRAYIS` runs a risk of hurting rather than helping; see the updated
tests.
@syssimics
Copy link
Copy Markdown
Contributor

syssimics commented Apr 5, 2026

PR Verification: ✅ success

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@mandolaerik mandolaerik Awaiting requested review from mandolaerik

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lwaern-intel @syssimics @mandolaerik