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 20916 - Print trace for deprecations triggered inside templates #11839

Merged
merged 5 commits into from
Oct 15, 2020

Conversation

Geod24
Copy link
Member

@Geod24 Geod24 commented Oct 9, 2020

This turned out to be way easier than I expected.
@John-Colvin : Want to give this a shot ?

I also think it segv dtoh (and I have an idea where the bug is), but let's see what the auto-tester says...

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @Geod24!

Bugzilla references

Auto-close Bugzilla Severity Description
20916 normal hard to find where a deprecation comes from

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

@Geod24
Copy link
Member Author

Geod24 commented Oct 9, 2020

@ibuclaw @kinke : Do you need to access printInstantiationTrace from C++ ? If not, I'll make it extern(D) and final.
The issue is that Classification uses Color (which is an enum of int). dtoh does not like the way Classification is defined.

@kinke
Copy link
Contributor

kinke commented Oct 9, 2020

I'm almost certain LDC doesn't.

@Geod24 Geod24 force-pushed the fix-20916 branch 5 times, most recently from a9d9d7d to 9edb35b Compare October 11, 2020 09:54
@@ -5984,6 +5984,10 @@ extern (C++) class TemplateInstance : ScopeDsymbol
for (TemplateInstance cur = this; cur; cur = cur.tinst)
{
++n_instantiations;
// Set error here as we don't want it to depend on the number of
// entries that are being printed.
cur.errors = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

What caused this commit?

Copy link
Member Author

Choose a reason for hiding this comment

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

It just seems very wrong to have this set only on the templates we show (which can vary depending on -v).
errors has meaning semantic-wise, and the amount of frames we print should not affect the semantic of the code.
I didn't witness anything wrong that prompted the commit, it's just my observation of what should happen.
Note that I believe the diff on test/fail_compilation/deprecations.d (L9 in the old code) is due to this: We don't show the error message twice anymore.

Ideally this shouldn't be in this function, but at least this
is more correct now.
This can be useful as a general tool, e.g. for deprecations.
Also make the function `extern(D)` as the C++ header generation
currently does not handle the `enum` properly and generate nonsense
(https://issues.dlang.org/show_bug.cgi?id=21300).
@Geod24
Copy link
Member Author

Geod24 commented Oct 14, 2020

This should be good to go (there was a spurious Buildkite failure, re-triggered).

@Geod24 Geod24 requested a review from thewilsonator October 15, 2020 03:36
@dlang-bot dlang-bot merged commit 5cc283c into dlang:master Oct 15, 2020
@Geod24 Geod24 deleted the fix-20916 branch October 15, 2020 05:36
@ibuclaw
Copy link
Member

ibuclaw commented Oct 20, 2020

Well this pulls in excessive baggage to the compiler front-end.

dmd/dtemplate.d:5972:26: error: undefined identifier ‘Classification’
 5972 |     extern(D) final void printInstantiationTrace(Classification cl = Classification.error)
      |                          ^

Copying over Classification from dmd ad verbatim, but now there's a missing module.

dmd/errors.d:24:13: error: undefined identifier ‘Color’
   24 |     error = Color.brightRed,          /// for errors

This occurs in a non-shareable module, so not a big deal, but just highlighting that maybe an another enum would be better.

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.

6 participants