Fix up lexpos for symbols from empty productions -- SIMICS-23466#430
Fix up lexpos for symbols from empty productions -- SIMICS-23466#430lwaern-intel wants to merge 4 commits intointel:mainfrom
lexpos for symbols from empty productions -- SIMICS-23466#430Conversation
| name = t[3] | ||
| (inp, outp, throws) = ([], t[4], True) | ||
| if logging.show_porting: | ||
| i = min([4, 5, 6], key=lambda i: t.lexpos(i)) |
There was a problem hiding this comment.
I am a bit peeved that you knew about this but never created an issue about it. Not overly, though. My anger remains largely directed towards Ply.
There was a problem hiding this comment.
My idea was that someday we'll move on to a better parser, and then the problem will vanish for free; the accumulated cost of this shortcoming small until then. But the cost of maintaining your fix is probably smaller so that's a better choice.
There was a problem hiding this comment.
i feel compelled to reiterate that this bug has served as my personal sleep paralysis demon for years. It's a bit ironic that when you figured it out way-back-when you went "ehh, who cares." Guess you have your answer!
|
PR Verification: ✅ success |
There was a problem hiding this comment.
Pull request overview
Fixes incorrect source column/line reporting by ensuring tokens produced from empty grammar productions get a sane lexpos, and adds supporting API/test updates.
Changes:
- Add
tokenfuncwrapper to track the most recently lexed token during parsing. - Introduce
fixup_emptyprod_lexpos(t)and apply it to many empty grammar productions to correctlexpos/lineno. - Extend
Sitewithcolno, add release notes entry, and add a unit test module validating empty-production site behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| RELEASENOTES.md | Documents the fix for incorrect column/rare line reporting (SIMICS-23466). |
| py/dml/toplevel.py | Passes tokenfunc into PLY parser to enable empty-production lexpos fixup. |
| py/dml/logging.py | Adds Site.colno and implements it for relevant site types. |
| py/dml/dmlparse.py | Implements token tracking + empty-production lexpos fixup; applies fixup across grammar rules. |
| py/dml/dmlparse_test.py | Adds unit test coverage for sites derived from empty productions (but currently has issues). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
PR Verification: ✅ success |
|
yet again, Copilot shows its greatest strength to be a stupid mistake checker while also showing its lack of any deeper understanding. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 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.
|
PR Verification: ✅ success |
| return ast | ||
|
|
||
| class test_emptyprod_based_sites(unittest.TestCase): | ||
| def test(self): |
There was a problem hiding this comment.
this test will look arbitrary and strange to a newcomer unless you mention that it tests that the fixup function does its thing
There was a problem hiding this comment.
You don't think the name is sufficient? I'd figure searching "emptyprod" within dmlparse or even the test itself would immediately tell you what it's about.
There was a problem hiding this comment.
I wrote it instead as another commit, easier than arguing about it.
I realized that the test is somewhat brittle (if we somehow rewrite method and object without using empty production rules, then it will no longer cover anything). But whatever, let's ignore that.
|
|
||
| RUN_PY_UNIT_TEST := $(PYTHON) $(DMLC_DIR)/run_unit_tests.py | ||
| %-pyunit-tested: $(DMLC_DIR)/py/dml/%.py $(OUT_PYFILES) | ||
| %-pyunit-tested: $(DMLC_DIR)/py/dml/%.py $(OUT_PYFILES) $(OUT_GEN_PYFILES) |
There was a problem hiding this comment.
Explain this part cause this I don't get.
There was a problem hiding this comment.
ply's parse tables are built in a caching fashion: if a parse table is pre-built then it will be used, otherwise a new one will be built (and discarded). This change ensures that we wait for the pre-built table.
|
some suggested fixes here: lwaern-intel#3 |
Otherwise the parsetab may be built implicitly when dmlparse_test is run
Fixup function should not be called on nonempty rules Also rewrite as unittest
|
PR Verification: ✅ success |
@mandolaerik any suggestions on how to best test this? Y'know, besides
portingand #431.