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.

Sign up for GitHub

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

Jump to bottom

fix Issue 23145 - Stack allocation of scope new variables defeats @safe #14175

Merged
merged 1 commit into from
Feb 9, 2023
Merged

fix Issue 23145 - Stack allocation of scope new variables defeats @safe #14175

merged 1 commit into from
Feb 9, 2023

Conversation

WalterBright
Copy link
Member

Timon's idea on how to fix this - disallow constructors for stack allocation for operator new unless the constructor is scope.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
✓ 23145 major Stack allocation of scope new variables defeats @safe

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#14175"

thewilsonator
thewilsonator previously requested changes May 29, 2022
View reviewed changes
Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

This should target stable unless this is a master only problem.

@WalterBright
Copy link
Member Author

This is not a regression, the problem is as old as dip1000.

@kinke
Copy link
Contributor

kinke commented May 29, 2022

This is primarily a big functional change, definitely worth a big changelog entry. - E.g., I'm pretty sure this prevents all scope Visitor stack-allocation optimizations in the frontend itself. And requires even more attribute soup to get the previous behavior.

@WalterBright WalterBright force-pushed the fix23145 branch 2 times, most recently from a3da521 to 8d260c4 Compare May 30, 2022 00:45
@WalterBright
Copy link
Member Author

@kinke you're right. It'll probably have to go behind a preview switch.

@WalterBright
Copy link
Member Author

Or perhaps we should just start doing attribute inference for constructors.

@WalterBright WalterBright force-pushed the fix23145 branch 2 times, most recently from 92a528a to cae48c5 Compare May 30, 2022 01:39
@WalterBright
Copy link
Member Author

Let's see how far this gets before deciding the best way to deal with older code.

@WalterBright WalterBright mentioned this pull request May 30, 2022
Merged
ibuclaw
ibuclaw previously requested changes Jun 4, 2022
View reviewed changes
src/dmd/nogc.d Outdated Show resolved Hide resolved
@WalterBright WalterBright force-pushed the fix23145 branch 5 times, most recently from 4eb4fa6 to 5128101 Compare June 6, 2022 07:02
@WalterBright WalterBright dismissed stale reviews from thewilsonator and ibuclaw June 6, 2022 22:04

this is a master only problem

thewilsonator
thewilsonator previously requested changes Jun 7, 2022
View reviewed changes
Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

This is not a master only issue, please check before making such false assertions. This affects at least 2.099+ (checked on, run.dlang.io, trying all compilers results in a server error).

@WalterBright
Copy link
Member Author

I don't understand your point. Putting this in stable will destabilize stable.

ljmf00
ljmf00 previously requested changes Jul 23, 2022
View reviewed changes
compiler/test/compilable/test23145.d Outdated Show resolved Hide resolved
compiler/test/compilable/test23145.d Outdated Show resolved Hide resolved
compiler/src/dmd/dsymbolsem.d Show resolved Hide resolved
compiler/src/dmd/clone.d Show resolved Hide resolved
@ljmf00 ljmf00 added the Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org label Jul 23, 2022
@ljmf00
Copy link
Member

ljmf00 commented Jul 23, 2022

Adding Spec PR tag to clarify scope attribute applied to constructors.

RazvanN7
RazvanN7 approved these changes Aug 12, 2022
View reviewed changes
Copy link
Contributor

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

Mind the nit and please address the other reviewers concerns. @ibuclaw @Geod24 any other objections?

compiler/src/dmd/nogc.d
import dmd.aggregate;
import dmd.apply;
import dmd.astenums;
import dmd.declaration;
import dmd.dscope;
import dmd.errors;
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding these 2 imports seems to be an unrelated change.

Copy link
Member Author

Choose a reason for hiding this comment

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

people keep removing core.stdc.stdio, and I keep adding it back in, because it is for the debugging printf's.

Geod24
Geod24 reviewed Aug 30, 2022
View reviewed changes
Copy link
Member

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

Suggestion for the message

compiler/test/compilable/test23145.d Outdated Show resolved Hide resolved
@WalterBright WalterBright dismissed thewilsonator’s stale review February 9, 2023 04:54

definitely not for stable, as it introduces a deprecation

@WalterBright WalterBright dismissed ljmf00’s stale review February 9, 2023 05:39

responded to it

@WalterBright WalterBright merged commit 74e4017 into dlang:master Feb 9, 2023
@WalterBright WalterBright deleted the fix23145 branch February 9, 2023 07:29
@WalterBright WalterBright mentioned this pull request Jul 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RazvanN7 RazvanN7 RazvanN7 approved these changes

@ljmf00 ljmf00 ljmf00 left review comments

@Geod24 Geod24 Geod24 left review comments

@kinke kinke kinke left review comments

@ibuclaw ibuclaw ibuclaw left review comments

@benjones benjones benjones left review comments

@thewilsonator thewilsonator thewilsonator left review comments

Assignees
No one assigned
Labels
Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org Severity:Bug Fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@WalterBright @dlang-bot @kinke @ljmf00 @benjones @ibuclaw @Geod24 @thewilsonator @RazvanN7