Skip to content
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 20691 - Converting scope static array to scope dynamic arra… #10951

Merged
merged 1 commit into from
Aug 27, 2021

Conversation

WalterBright
Copy link
Member

…y should be error

@dlang-bot
Copy link
Contributor

dlang-bot commented Mar 21, 2020

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
20691 normal Converting scope static array to scope dynamic array should be error

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#10951"

@Geod24
Copy link
Member

Geod24 commented Mar 21, 2020

Added a link in 20691 to 20505 (why the duplication ??).
So do I understand correctly that the reasoning is that there can only be one level of scope-ness ?

@WalterBright WalterBright force-pushed the fix20691 branch 2 times, most recently from bee84dd to 897912a Compare March 22, 2020 05:46
@WalterBright
Copy link
Member Author

why the duplication ??

I originally thought it was a different problem.

the reasoning is that there can only be one level of scope-ness ?

Yes. Scope is not transitive.

@WalterBright
Copy link
Member Author

Unfortunately, several projects in buildkite are relying on this compiler bug.

@Geod24
Copy link
Member

Geod24 commented Mar 22, 2020

Yes. Scope is not transitive.

Then why do struct allow one to store a scope slice in them ?

Also, can you renamed the commit title to Fix issue 20505 - [...], since 20691 was a duplicate ?

@Geod24
Copy link
Member

Geod24 commented Mar 22, 2020

Unfortunately, several projects in buildkite are relying on this compiler bug.

We discussed this a million time, but I'm going to repeat it anyway: anyone trying to do something useful with scope (that is, past identity functions) expect it to be transitive.
If scope is not transitive, then how do we deal with struct storing slices ?

I tested this PR with some sample code, and this still compiles:

import std.stdio;

struct Bar
{
    const(char)[] v;
}

struct Store
{
    Bar[] v;
}

void main () @safe
{
    auto v = func1();
    writeln(v);
}

const(char)[] func1 () @safe
{
    char[10] str = 'a';
    return func2(str);
}

const(char)[] func2 (scope const(char)[] b) @safe
{
    Bar[10] array = Bar(b);
    return func3(array, b);
}

const(char)[] func3 (scope Bar[] array, scope const(char)[] b) @safe
{
    Store st = Store(array);
    return st.v[0].v;
}

@Geod24
Copy link
Member

Geod24 commented May 15, 2020

Any update on this ?

@WalterBright
Copy link
Member Author

@Geod24 this still compiles

please add bug report.

@Geod24
Copy link
Member

Geod24 commented May 30, 2021

please add bug report.

I did, it's the one you are trying to fix (https://issues.dlang.org/show_bug.cgi?id=20505).

@WalterBright
Copy link
Member Author

@Geod24 as for transitive scope, while a good idea, that would be a fairly major project, way beyond the scope of this PR. It is likely to also break much existing code.

@Geod24
Copy link
Member

Geod24 commented May 30, 2021

@WalterBright :

  1. I agree with you, having transitive scope would be a fairly major undertaking, and TBH I'm not sure it would allow us to have @safe RC either;

  2. By definition, code that is triggered by a -preview is experimental. So at the moment, whatever change we decide to go with, as long as it only triggers when -preview=dip1000 is used, it's not a breaking change;

I'll merge the druntime PR in the meantime.

@dkorpel
Copy link
Contributor

dkorpel commented Jul 2, 2021

Now that the blocker from Phobos has been fixed, can this be merged? I assume the failing test runs are old and should go away after a rebase.

@dkorpel
Copy link
Contributor

dkorpel commented Jul 6, 2021

@WalterBright @RazvanN7 Can you rerun the tests?

@@ -12490,13 +12490,15 @@ bool checkAddressVar(Scope* sc, Expression exp, VarDeclaration v)
v.storage_class &= ~STC.maybescope;
v.doNotInferScope = true;
if (exp.type.hasPointers() && v.storage_class & STC.scope_ &&
!(v.storage_class & STC.temp) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this for?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a remnant of the old bug that caused enclosesLifetimeOf to return wrong results for VarDeclarations created during semantic?

@dkorpel dkorpel added dip1000 memory safety with scope, ref, return Review:Vision Vision Plan https://wiki.dlang.org/Vision/2018H1 Review:Needs Rebase labels Jul 16, 2021
@dkorpel
Copy link
Contributor

dkorpel commented Jul 16, 2021

Ping @WalterBright @RazvanN7

@WalterBright
Copy link
Member Author

Looks like the problem is in phobos:

std/process.d(1960): Error: cannot take address of `scope` local `args` in `@safe` function `spawnShell`
std/process.d(1960): Error: cannot cast expression `args` of type `const(char)[][3]` to `const(char[])[]`

@dkorpel
Copy link
Contributor

dkorpel commented Jul 27, 2021

Looks like the problem is in phobos:

So the error is still there. I thought it would be fixed by dlang/phobos#8119

@thewilsonator
Copy link
Contributor

Removing auto merge this has failures all across build kite

@dkorpel
Copy link
Contributor

dkorpel commented Aug 27, 2021

The buildkite failures were in projects not using dip1000, so I added an if (global.params.useDIP1000 == FeatureState.enabled) and it's green now.

@dlang-bot dlang-bot merged commit b838466 into dlang:master Aug 27, 2021
@WalterBright WalterBright deleted the fix20691 branch February 6, 2022 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dip1000 memory safety with scope, ref, return Merge:auto-merge Review:Vision Vision Plan https://wiki.dlang.org/Vision/2018H1 Severity:Bug Fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants