Skip to content

Fix bugzilla 24680 - [dip1000] final auto class method infers scope b…#16751

Merged
dkorpel merged 2 commits intodlang:masterfrom
dkorpel:escape-class-final
Jul 29, 2024
Merged

Fix bugzilla 24680 - [dip1000] final auto class method infers scope b…#16751
dkorpel merged 2 commits intodlang:masterfrom
dkorpel:escape-class-final

Conversation

@dkorpel
Copy link
Contributor

@dkorpel dkorpel commented Jul 24, 2024

…ut no return

Likely for historical reasons, it's possible to enable scope inference but disable return inference, which breaks escape.d in the case of the bug report. Unify them under scopeInprocess to be consistent with nogcInprocess etc., and to disambiguate it from the function inferScope(VarDeclaration).

@dkorpel dkorpel added the dip1000 memory safety with scope, ref, return label Jul 24, 2024
@dkorpel dkorpel requested a review from ibuclaw as a code owner July 24, 2024 19:14
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @dkorpel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
24680 normal [dip1000] final auto class method infers scope but no return

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

bool scopeInprocess; /// infer `return` and `scope` for parameters
bool inlineScanned; /// function has been scanned for inline possibilities
bool inferScope; /// infer 'scope' for parameters
bool dummy; /// unused
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you wonder 'Why not just remove this?', you're in for a treat. Follow up PR coming soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can be removed after #16752 is merged

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe not if older compilers are affected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Between versions, there's no expectation of binary compatibility of AST nodes right?

@dkorpel
Copy link
Contributor Author

dkorpel commented Jul 29, 2024

Ping @RazvanN7

@dkorpel dkorpel force-pushed the escape-class-final branch from ba01f81 to 936155f Compare July 29, 2024 14:11
@dkorpel dkorpel merged commit 8d1218e into dlang:master Jul 29, 2024
@dkorpel dkorpel deleted the escape-class-final branch July 29, 2024 15:22
thewilsonator pushed a commit to thewilsonator/dmd that referenced this pull request Oct 7, 2024
dlang#16751)

* Fix bugzilla 24680 - [dip1000] final auto class method infers scope but no return

* Remove dummy variable
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 Severity:Bug Fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants