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 15984 [REG2.071] Interface contracts retrieve garbage instead of parameters #9030

Closed
wants to merge 3 commits into from

Conversation

thewilsonator
Copy link
Contributor

The LDC fix is essentially this (c.f. LDC's fix)

although I'm not too sure about parents and dealing with irs.

cc @rainers @WalterBright @FeepingCreature

@dlang-bot
Copy link
Contributor

dlang-bot commented Dec 3, 2018

Thanks for your pull request and interest in making D better, @thewilsonator! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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
15984 regression [REG2.071] Interface contracts retrieve garbage instead of parameters

⚠️⚠️⚠️ 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 fetch digger
dub run digger -- build "master + dmd#9030"

@@ -1363,6 +1363,32 @@ elem *toElem(Expression e, IRState *irs)
FuncDeclaration fd = te.var.toParent2().isFuncDeclaration();
assert(fd);
ethis = getEthis(te.loc, irs, fd);

/* Bugzilla 15984: If te is in interface contracts, ethis is a
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, I would quote any "word" that references a symbol in the code, to avoid any confusion with a regular word that is misspelled.

Copy link
Member

Choose a reason for hiding this comment

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

Please use:

https://issues.dlang.org/show_bug.cgi?id=15984

instead of:

Bugzilla 15984

@Geod24
Copy link
Member

Geod24 commented Dec 3, 2018

Please use more descriptive commit title (and as a consequence, PR title).

@FeepingCreature
Copy link
Contributor

Note that this does not actually fix 15984.

import core.stdc.stdio;
interface I {
    void foo(Object obj)
    in { printf("%p\n", obj); obj.toString; }
}
class C : I { override void foo(Object obj) in { } do { } }
void main()
{
    auto c = new C;
    c.foo(new Object);
}

still fails with the branch.

@thewilsonator
Copy link
Contributor Author

I'm not expecting that to, given the CI is red ;), I'm not sure which FunctionDeclarations are which.

{
if (auto iface = parentfd.parent.isInterfaceDeclaration())
{
Type thistype = irs.getFunc().vthis.type;
Copy link
Contributor Author

@thewilsonator thewilsonator Dec 6, 2018

Choose a reason for hiding this comment

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

@FeepingCreature In particular I'm not certain about the provenance of irs.getFunc() (c.f. gIR->func() and fd (c.f.dfnval->func)
everything else I'm pretty sure is correct.

@WalterBright WalterBright changed the title Fix 15984 Fix 15984 [REG2.071] Interface contracts retrieve garbage instead of parameters Dec 7, 2018
@rainers
Copy link
Member

rainers commented Dec 9, 2018

Don't you have to fix the this pointer everywhere getEthis is called? So maybe move the fixup there?

@thewilsonator
Copy link
Contributor Author

Thanks I'll try that.

@thewilsonator
Copy link
Contributor Author

So, it turns out that it is already handled in getEthis, but obviously that doesn't work properly.

@thewilsonator thewilsonator deleted the fix-15984 branch May 15, 2019 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants