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 23241 - Consider types with no symbol (e.g. int) to have no… #14298

Merged
merged 2 commits into from
Jul 27, 2022

Conversation

maxhaton
Copy link
Member

… attributes

not completely convinced about this

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @maxhaton!

Bugzilla references

Auto-close Bugzilla Severity Description
23241 blocker __traits getMember breaks compilation when hit an alias

⚠️⚠️⚠️ 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#14298"

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

This is a blocker, should it target stable?

@BorisCarvajal
Copy link
Member

I think the trait should accept anything semantically correct and just return an empty tuple if no uda is available,
it would avoid this repetitive pattern:

static if (__traits(compiles, __traits(getAttributes, arg)))
	alias udas = __traits(getAttributes, arg);

compiler/src/dmd/traits.d Outdated Show resolved Hide resolved
compiler/src/dmd/traits.d Outdated Show resolved Hide resolved
Copy link
Contributor

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

This patch seems to address a symptom of a larger issue: eager substitution of alias. For example, this patch ignores the UDA for:

@("hi2") alias MyEnum = int;

Maybe semanticTiargs can be modified as to not substitute the alias in this situation. However, this may be complicated since you may have an alias to a symbol that also has an UDA. I think that getAttributes should only get the UDA of the alias, not for the aliased symbol.

@maxhaton
Copy link
Member Author

This patch seems to address a symptom of a larger issue: eager substitution of alias. For example, this patch ignores the UDA for:

@("hi2") alias MyEnum = int;

Maybe semanticTiargs can be modified as to not substitute the alias in this situation. However, this may be complicated since you may have an alias to a symbol that also has an UDA. I think that getAttributes should only get the UDA of the alias, not for the aliased symbol.

That's a much bigger change of behaviour that would probably break quite a lot of code if done wrong

@maxhaton
Copy link
Member Author

This patch seems to address a symptom of a larger issue: eager substitution of alias. For example, this patch ignores the UDA for:

@("hi2") alias MyEnum = int;

Maybe semanticTiargs can be modified as to not substitute the alias in this situation. However, this may be complicated since you may have an alias to a symbol that also has an UDA. I think that getAttributes should only get the UDA of the alias, not for the aliased symbol.

@RazvanN7 I have fixed the code. Please remove your requested changes as it has nothing to do with this PR and constitutes a large change in behaviour which would have to be decided somewhere else.

Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

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

I offer a suggestion, and still it's spelled wrong.

compiler/src/dmd/traits.d Outdated Show resolved Hide resolved
Co-authored-by: Iain Buclaw <ibuclaw@gdcproject.org>
@RazvanN7
Copy link
Contributor

Well, I am not convinced that this is the right fix. It seems to me that this PR is like giving painkillers to a cancer patient. Sure, it makes it better, but it's not actually addressing the real issue. However, I don't want to block this if others agree that it should get in.

@maxhaton
Copy link
Member Author

@ibuclaw

@PetarKirov PetarKirov dismissed ibuclaw’s stale review July 24, 2022 19:22

Requested change applied

Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

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

Looks good me. Even if the right thing to do is not resolve aliases too early, this change is still a step in the right direction as __traits(getAttributes, int) should return an empty value sequence, instead of failing to compile (expressed differently: all types should be symbols, including built-in ones).

@PetarKirov PetarKirov added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Jul 24, 2022
@RazvanN7 RazvanN7 merged commit 4b4975f into dlang:master Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge:72h no objection -> merge The PR will be merged if there are no objections raised. Severity:Bug Fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants