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 issue 15069 - nonsense struct template instantiations still compile #10613

Merged
merged 1 commit into from Nov 25, 2019
Merged

fix issue 15069 - nonsense struct template instantiations still compile #10613

merged 1 commit into from Nov 25, 2019

Conversation

ghost
Copy link

@ghost ghost commented Nov 24, 2019

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @StianGulpen! 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
15069 regression [REG2.064] nonsense struct template instantiations still compile

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 "stable + dmd#10613"

@ghost
Copy link
Author

ghost commented Nov 24, 2019

Sorry I had modified the original test case. The problem is still there.

@ghost ghost closed this Nov 24, 2019
@ghost
Copy link
Author

ghost commented Nov 25, 2019

I've found the real problem this time. An alias is created for each template parameter and this way the compiler thinks that it can be the enclosing template.

@ghost ghost reopened this Nov 25, 2019
src/dmd/dtemplate.d Outdated Show resolved Hide resolved
@ghost ghost requested a review from ibuclaw as a code owner November 25, 2019 10:52
@ghost
Copy link
Author

ghost commented Nov 25, 2019

ping for merge, this is good now. BTW there's also #10571.

@dlang-bot dlang-bot merged commit 94e56d7 into dlang:stable Nov 25, 2019
@ghost ghost deleted the issue-15069 branch November 25, 2019 13:02
@Geod24
Copy link
Member

Geod24 commented Nov 26, 2019

This doesn't look like a good solution to be fair. Sure it works, but having a boolean just to signal that this alias is not exactly like other aliases just means the abstraction was either wrong or incomplete to begin with.

The fact that template parameters are aliases in the scope has been problematic from time to time. For example, most of the complexity in the C++ mangling code on POSIX comes from that.

It's too minor to be worth a revert, but wanted to point it out in case you want to explore this further @StianGulpen .

@ghost
Copy link
Author

ghost commented Nov 26, 2019

I think about a refactoring in master after the next patch release. Instead of the field, new classes (or at least one, derived from AliasParameter) would be added. The information carried by the field would be available by the type of the class(es) (e.g isTemplateParameterAlias()). This new classes could be use to work on the other problems you mentioned earlier and that I was not aware of.

The idea is

class TemplateParameterAlias : AliasDeclaration {}

Honestly, knowing how things are generally well decomposed in DMDFE I was surprised that not utility exists allowing to make the difference, before my fix.

@ghost
Copy link
Author

ghost commented Nov 26, 2019

well TemplateParameterAlias is maybe too near from AliasTemplateParameter but you got the idea I think.

@Geod24
Copy link
Member

Geod24 commented Nov 26, 2019

This new classes could be use to work on the other problems you mentioned earlier and that I was not aware of.

I originally tried to solve the problem at the root when I encountered it, using a similar approach, but aliases are removed as early as possible, so adding a new class won't help, and refactoring the code to prevent aliases being eliminated is a daunting task. And if you manage to get the job done, you're going to have to convince the reviewers that your hundreds of lines of refactoring are adding value.

Another trivial example:

void main () { bar(42); }
alias Foo = Object;
void bar (immutable(char)[] arg) {}
void bar (Foo arg) {}

This outputs:

hack.d(1): Error: none of the overloads of bar are callable using argument types (int), candidates are:
hack.d(3):        hack.bar(string arg)
hack.d(4):        hack.bar(Object arg)

As you can see, the alias is bypassed and the suggestion uses Object as parameter, not Foo. But funnily, it shows string while I typed in immutable(char)[] ! That's because there is a special handling somewhere in DMD to work around this problem of aliases being too transparent.

@ghost
Copy link
Author

ghost commented Nov 26, 2019

Your example is not related to template params being pulled in the scope as aliases. It rather illustrates a problem of a special case with unexpected side effects. Special cases are evils.

@Geod24
Copy link
Member

Geod24 commented Nov 26, 2019

Your example is not related to template params being pulled in the scope as aliases.

Correct, but it is another symptom of the early elimination of aliases.

Special cases are evils.

👍
IMO this is a perfect example of how they just shift the problem elsewhere.

@ghost
Copy link
Author

ghost commented Nov 26, 2019

Ok we agree finally.

tramker pushed a commit to tramker/dmd that referenced this pull request Jul 24, 2020
fix issue 15069 - nonsense struct template instantiations still compile
merged-on-behalf-of: Nicholas Wilson <thewilsonator@users.noreply.github.com>

Signed-off-by: Martin Krejcirik <mk@krej.cz>
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.

3 participants