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 getSymbolsByUDA by replacing broad __traits(compiles) with a more narrow condition #8631

Merged
merged 1 commit into from Nov 28, 2022

Conversation

RazvanN7
Copy link
Collaborator

This was discovered in dlang/dmd#14554.

hasUDA uses getUDAs underneath which in turn uses traits(getAttributes). Up until dmd PR 14554 getUDAs would happily accept an overload set and because of the bug in __traits(getAttributes) it would just check the first lexicographically overload encountered.

getSymbolsByUDA which uses hasUDA internally would first check if hasUDA is callalble on the members of the passed aggregate by using __traits(compiles, hasUDA!member). This was done to prevent calling traits(getAttributes) on types or alias sequences, however, with the new deprecation from dmd PR 14554 and since phobos compiles with -de the __traits(compiles) check now fails for functions that have overloads, thus breaking getSymbolsByUDA.

The fix is to narrow the condition by specifically checking for types and alias sequences in the internals of getSymbolsByUDA, which this PR does, thus unblocking dmd PR 14554.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @RazvanN7! 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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 + phobos#8631"

@RazvanN7 RazvanN7 changed the title Fix getSymbolsByUDA by replacing broad __traits(compiles) with a more focused condition Fix getSymbolsByUDA by replacing broad __traits(compiles) with a more narrow condition Nov 25, 2022
@RazvanN7
Copy link
Collaborator Author

cc @dkorpel

Comment on lines +8852 to +8855
static if (Args.length != 1)
enum isAliasSeq = true;
else
enum isAliasSeq = false;
Copy link
Member

Choose a reason for hiding this comment

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

You know, enum isAliasSeq = (Args.length != 1) works too ;)

@Geod24 Geod24 merged commit e305dc9 into dlang:master Nov 28, 2022
@quickfur
Copy link
Member

This change unfortunately caused a regression: https://issues.dlang.org/show_bug.cgi?id=23776

@JohanEngelen
Copy link
Contributor

Possibly introduced another regression: https://issues.dlang.org/show_bug.cgi?id=23977

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants