fix Issue 16301 - CTFE execution of opApply keeps wrong "this" context in foreach's body #6813
Conversation
|
Please use a more descriptive title when submitting P.R., so that people watching this repo don't have to open the P.R. to know if it's of interest to them. |
|
|
@mathias-lang-sociomantic |
|
@WalterBright |
UplinkCoder
force-pushed
the
fix_16301
branch
2 times, most recently
from
097b742
to
6a15ae1
Compare
WalterBright
changed the title
|
When doing bugfix PRs, please have the subject match the bugzilla entry, prefixed with "fix". I already fixed this one. |
The same goes for the commit messages... |
I added the [Needs Work] tag instead. |
|
@WalterBright I think the work is done. |
…t in foreach's body
|
There is almost certainly matching code in glue/e2ir/etc. Can that be re-used somehow, or can the context be cached during semantic? This will ensure the lookup has the same result at runtime and ctfe. |
|
@yebblies I had a look at e2ir and it add a negative offset to the stack-ptr, to walk up the stack-frames, we cannot do that in ctfe. |
|
But isn't that pretty much what we're doing here? We do have a stack and we can index with a negative offset to our current frame...? Your code here to walk up the scope chain and find the right 'this', which I assume e2ir also does, could that be moved into semantic somewhere? Or does semantic already do this (for semantic checking) in which case can it save the result somewhere so we don't have to recompute? |
|
@yebblies the |
| // a this that has a matching type. | ||
| // https://issues.dlang.org/showbug.cgi?id=16301 | ||
| size_t stackOffset = ctfeStack.savedThis.dim; | ||
| while(stackOffset && (!result || !result.type.equals(e.type))) |
There was a problem hiding this comment.
Isn't this essentially implementing a dynamic scoping approach instead of a lexical scoping one?
If ctfeStack is the call stack - this sounds incorrect in the general case.
There was a problem hiding this comment.
That's right, ctfeStack is the call stack.
However if a there is a closure it must have been created by a function call, therefore
The aggregate's this must be found and will be the same.
If you look at e2ir's closure handling, it's doing the same thing.
Lexical scope does not matter it just happens to be the same.
|
@EyalIO has proven this unsound. |
Fixes issue 16301 by walking up the stack of previous this-pointers and looking for a match.
This will allow delegates to properly work at ctfe.