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 13667 - ICE: stack overflow using self-referencing cast ins… #12168

Merged
merged 1 commit into from
Feb 15, 2021
Merged

Fix Issue 13667 - ICE: stack overflow using self-referencing cast ins… #12168

merged 1 commit into from
Feb 15, 2021

Conversation

BorisCarvajal
Copy link
Member

…ide recursive alias this method

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @BorisCarvajal! 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
✓ 13667 critical ICE: stack overflow using self-referencing cast inside recursive alias this method

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

To target stable perform these two steps:

  1. Rebase your branch to upstream/stable:
git rebase --onto upstream/stable upstream/master
  1. Change the base branch of your PR to stable

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

@RazvanN7
Copy link
Contributor

RazvanN7 commented Feb 1, 2021 •
edited
Loading

Please target stable, otherwise LGTM.

@RazvanN7 RazvanN7 added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Feb 1, 2021
@BorisCarvajal
Copy link
Member Author

Can it be merged to master? Another alias this fix (#12161) is there and I'm gonna push a little refactoring to replace with a function the same 4 lines of code before every alias this resolution.

WalterBright
WalterBright previously requested changes Feb 3, 2021
View reviewed changes
Copy link
Member

@WalterBright WalterBright left a comment

Choose a reason for hiding this comment

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

Please don't mix refactoring with bug fixes. First do one, then the other.

src/dmd/dcast.d Show resolved Hide resolved
src/dmd/dcast.d Outdated
@@ -1508,6 +1487,32 @@ Expression castTo(Expression e, Scope* sc, Type t)
this.t = t;
}

// Try casting the alias this member. Return the expression if it succeeds, null otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

What are tob and t1b ?

Copy link
Member

Choose a reason for hiding this comment

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

Am I correct that moving tryAliasThisCast here was just a refactoring, and not part of the fix? If so, refactorings (no matter how tempting) should go in separate PRs.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is a refactoring, but it also adds some code.

Copy link
Contributor

Choose a reason for hiding this comment

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

But you are right, the refactoring does make it difficult to spot the added bits.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep sorry, I've separated refactoring and fix in two commits. It looks more clear now.

src/dmd/opover.d Show resolved Hide resolved
@WalterBright WalterBright removed the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Feb 3, 2021
RazvanN7
RazvanN7 reviewed Feb 3, 2021
View reviewed changes
src/dmd/dcast.d Outdated
if (att1 && t1b.equivalent(att1))
return null;
else if (!att1 && t1b.checkAliasThisRec())
att1 = t1b;
Copy link
Contributor

Choose a reason for hiding this comment

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

@WalterBright Lines 1501-1504 were not previously in this function.

Copy link
Member Author

Choose a reason for hiding this comment

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

These are the check needed before any alias this resolving to avoid infinite recursion.

@WalterBright
Copy link
Member

Bug fixes and refactorings should be in separate PRs, not just separate commits. The reason is so they can be reviewed independently on their merits, as well as the usual if one introduces a regression, it doesn't implicate the other.

I remember a PR from a few years ago that was shown to be responsible for a regression. The code was around 250 lines of complicated refactoring, and one line of bug fix. It took me a long time to figure that out.

@BorisCarvajal
Copy link
Member Author

Ok

@BorisCarvajal
Copy link
Member Author

Refactoring -> #12180

@RazvanN7 RazvanN7 mentioned this pull request Feb 8, 2021
Merged
@RazvanN7
Copy link
Contributor

Rebase and we're good to go.

@RazvanN7
Copy link
Contributor

@WalterBright Do you have any other objections on this PR?

@RazvanN7 RazvanN7 dismissed WalterBright’s stale review February 15, 2021 14:13

Review was addressed.

@RazvanN7 RazvanN7 mentioned this pull request Feb 15, 2021
Merged
@dlang-bot dlang-bot merged commit 4ec62c1 into dlang:master Feb 15, 2021
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

@WalterBright WalterBright WalterBright left review comments

Assignees
No one assigned
Labels
Merge:auto-merge Severity:Bug Fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@BorisCarvajal @dlang-bot @RazvanN7 @WalterBright