Modify MustUse to iterate the stack frame and collate options from all frames. #1621
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1621 +/- ##
=======================================
Coverage 91.24% 91.24%
=======================================
Files 44 44
Lines 11394 11411 +17
Branches 2223 2226 +3
=======================================
+ Hits 10396 10412 +16
Misses 836 836
- Partials 162 163 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I'm undecided on this change overall. I agree that a better way to manage the noise from UnusedElaboratable is needed, but I'm not sure this is the best approach.
The main problem that gives me pause is that the semantics is so vaguely defined. Why does the outermost frame take priority, for example? Why should we not just follow the super() call chain in the constructor instead of going through the entire callstack?
I don't want to let this PR rot but I do want at least some discussion of the exact semantics to happen first. People will come to rely on it, and changing it later will become difficult.
…l frames. Options in closer frames take precedence
robtaylor
force-pushed
the
better-linter-options
branch
from
c44ec87 to
ad6569e
Compare
|
Oh, the other potential problem is that by capturing the callstack you will prevent any of the objects referenced by any of the frames from being collected before the entire design is garbage-collected. I think this doesn't matter for batch applications, but it will probably bite someone pretty hard eventually. |
The reasoning here is that it would be the user's code closest to the error that would control the output, which seemed the most logical choice at the time. but it may indeed make more sense that its the 'top level' that controls it.
agreed. Its not obvious what the best policy is. That said, I think on reflection that following the call chain of the constructor could be confusing as the 'point' where a user gets the list of warnings may be quite separate in space and time from any given amaranth object. |
|
oh, good catch. Does it work to do the work of checking options at MustUse
creation instead of at __del__?
…On Mon, 4 Aug 2025 at 23:45, Catherine ***@***.***> wrote:
*whitequark* left a comment (amaranth-lang/amaranth#1621)
<#1621 (comment)>
Oh, the other potential problem is that by capturing the callstack you
will prevent any of the objects referenced by any of the frames from being
collected before the entire design is garbage-collected. I think this
doesn't matter for batch applications, but it will probably bite someone
pretty hard eventually.
—
Reply to this email directly, view it on GitHub
<#1621 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB4O4H4RWWZ75U4MEMQ7TD3L7PBJAVCNFSM6AAAAACCXVJZBSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCNJSGY2DSMZZGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
At the time I implemented the existing code, I was concerned about the overhead of using A compromise could be to turn a traceback into a list of |
|
By the way, regarding the "first/last wins" discussion: one caveat is that the same file can appear in the traceback multiple times, potentially in between two appearances of some other file; this is something that will be particularly acute if you use decorators. |
I was thinking this too, so I measured it - surprisingly it makes little difference over the amaranth test suite, i guess as most of the sources are in local cache. Comparing here main (first set of data) to 5d82323 <style type="text/css"></style>
|
Options in closer frames take precedence