-
Notifications
You must be signed in to change notification settings - Fork 440
Dyno: Fix POI merging #28226
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
Merged
Merged
Dyno: Fix POI merging #28226
+190
−76
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
a5b40d9 to
a3ef76f
Compare
3 tasks
arifthpe
approved these changes
Jan 6, 2026
DanilaFe
added a commit
that referenced
this pull request
Jan 6, 2026
Depends on #28243 only for convenience. In investigating the bug fixed by #28226, I noticed that some of our module code relied on POI where it probably didn't intend to do so (or at least, where it seemed brittle). I adjusted the compiler to identify such cases, and found a number of generic functions that claimed to use POI that weren't actually using POI. It turned out that Dyno was considering methods found via forwarding to be "POI". Besides being inaccurate, this tainted the resulting `PoiInfo` of the resolved function, thereby reducing its eligibility for being used in caching. E.g., any method that (transitively) relied on DSI methods (e.g., creating array literals, copying arrays, array indexing) was marked as "non-cache-eligible" because it "relied on POI" (even though it just used forwarding to the array instance). It's not clear to me how impactful this issue was, since many calls remain uncacheable after this PR because they contain recursion. However, I did confirm that _some_ uncacheable calls became cacheable after this PR. Thus, we are bound to see an improvement, if a tiny one. ## Testing - [x] dyno tests - [x] paratest `--dyno-resolve-only` - [ ] paratest
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Encountering the same candidate N times through a PoI scope should not baloon the number of reported rejected candidates. Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
814b0ab to
f40aa3e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Consider the program:
The three->way chain is required, because this bug is about collapsing POI scopes.
Basically, by POI, this program resolves:
a(...)is resolved in the scope ofb(…), which is resolved in the scope ofc(…), which has aprivate use Lib. Thus,a(…)has access tooutermost(…).However, we try to avoid building up long chains of POI scopes (a-in-scope-of-b-in-scope-of-c). As an optimization, if one module uses another, and both are in the POI chain, we remove the usee from the POI chain, since anything in its scope will be found via the use. In this case,
BusesC, so we removeCfrom the POI scope lookup chain. Unfortunately, this is not sound, asChas private use ofLib, which does not propagate throughuse C. As a result, the call toamissesoutermost.This PR adjusts the logic of
isWholeScopeVisibleFromScope, which is what used to implement the optimization. In particular, I found it to have two odd behaviorsprivate useissue outlined above: not caring about definitions brought in privately (and thus, not re-exported)use MParent.MChild, whereMParenthad apublic use Lib, was treated as findingLib. However, I don't believe this is accurate. See:This PR reimplements
isWholeScopeVisibleFromScopefrom first principles. I consider two ways for one scope to be wholly visible from another:use Mcan see all ofMbut only ifMdoesn't have private definitions. For example, ifMhas aprivate proc,use Mwill not bring that in, and thus, it's not true that the scope ofMis "fully visible" from the user.use Mcan be transitive; either there's ause Mright in the scope, OR there's ause Intermediate, whereIntermediatehas apublic use M, ORIntermediatehas apublic use OtherIntermediatewhich has apublic use M.... Note that except for the firstuse, all uses must bepublic.To help reduce the impact of this method, I kept it conservative. As a result, I expect
isWholeScopeVisibleFromScopeto now be stricter than it used to be (certainly it is in the buggy case). This technically expanded existing POI chains in some cases and might've affected caching of instantiations. However, I observed no noticeable performance impact from that, either.Reviewed by @arifthpe -- thanks!
Testing