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
fix Issue 17784 - [scope][DIP1000] Confusing error message for escapi… #8054
Conversation
Thanks for your pull request, @WalterBright! Bugzilla references
|
7cb3e08
to
f6800ba
Compare
if (global.params.vsafe) // https://issues.dlang.org/show_bug.cgi?id=17029 | ||
{ | ||
if (!gag) | ||
error(e.loc, "scope variable `%s` may not be copied into allocated memory", v.toChars()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using the term "GC-allocated memory" to be more precise. The problem isn't about memory (per se) or its allocation but about the lifetimes of objects under the jurisdiction of the GC. I suggest something like:
scope variable
%s
may not be copied into GC-allocated memory because doing so would cause it to escape
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't use that term because the message could apply to any allocated memory.
We run the risk of turning D into Rust, where instead of users fighting the borrow checker, they'll be fighting the scope checker. The wording of the error messages will become increasingly important to help users understand precisely what the problem is. |
if (tb.ty == Tarray || tb.ty == Tsarray) | ||
{ | ||
if (!gag) | ||
error(e.loc, "copying `%s` into allocated memory escapes a reference to variadic parameter `%s`", e.toChars(), v.toChars()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion:
copying
%s
into GC-allocated memory escapes a reference to variadic parameter%s
if (!gag) | ||
{ | ||
const(char)* kind = (v.storage_class & STC.parameter) ? "parameter" : "local"; | ||
error(e.loc, "copying `%s` into allocated memory escapes a reference to %s variable `%s`", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest:
copying
%s
into GC-allocated memory escapes a reference to %s variable%s
src/dmd/escape.d
Outdated
if (tf.isref) | ||
{ | ||
if (!gag) | ||
error(e.loc, "storing reference to outer local variable `%s` into allocated memory", v.toChars()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand what the intention is, but if my understanding is correct I suggest:
storing a reference to outer local variable
%s
into GC-allocated memory causes it to escape
src/dmd/escape.d
Outdated
{ | ||
if (log) printf("byexp %s\n", ee.toChars()); | ||
if (!gag) | ||
error(ee.loc, "storing reference to stack allocated value returned by `%s` into allocated memory", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest:
storing a reference to a stack allocated value returned by
%s
into GC-allocated memory causes it to escape
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add additional tests to increase coverage.
In principle I agree with @JinShil, but I think that the source of the momery allocation (e.g. GC vs malloc vs static) doesn't matter, so it's better to simply say "storing / copying [..] to an object with greater lifetime". |
…ng local via new-expression
I tend to think of lifetimes in relationship to lexical scopes, so I prefer using "allocated memory" instead. |
…ng local via new-expression
This splits off
checkNewEscape()
fromcheckReturnEscapeImpl()
which enables customizing the error messages and removes confusing logic trying to deal with both Return and New.