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 21570 - Deprecate __traits(isStaticArray, ...) accepts enums... #12142

Conversation

MoonlightSentinel
Copy link
Contributor

@MoonlightSentinel MoonlightSentinel commented Jan 21, 2021

...with static array as base type.

The spec[1] states that a named enum creates a distinct type which is
implicitly convertible to it's base type. Hence __traits(isStaticArray, <enum>)
should yield false as done for an equivalent is(...) expression.

[1] https://dlang.org/spec/enum.html#named_enums

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 requires a changelog entry and a deprecation as it is silently changing behavior. Looks good otherwise.

@RazvanN7
Copy link
Contributor

RazvanN7 commented Jan 22, 2021

Also, it looks like there are some public examples that need to be updated.

Later edit: Hm... it seems that this might not be a bug? Look at this documentation example [1].

From the is expression perspective those are different types, however, it seems that traits(isStaticArray)
considers the enum just a shell. This might be the intended behavior.

[1] https://dlang.org/spec/traits.html#isStaticArray

@MoonlightSentinel
Copy link
Contributor Author

This requires a changelog entry and a deprecation as it is silently changing behavior. Looks good otherwise.

How should that deprecation work? Retain the current behaviour and issue a deprecation message for enums?

Later edit: Hm... it seems that this might not be a bug? Look at this documentation example [1].

Those examples were added long after the introduction of isStaticArray to document the current behaviour (see dlang/dlang.org#2507). But if this is intended behaviour it should be documented more obviously, especially because of the missmatch to is(...).

@RazvanN7
Copy link
Contributor

RazvanN7 commented Jan 22, 2021

How should that deprecation work? Retain the current behaviour and issue a deprecation message for enums?

Yes.

Those examples were added long after the introduction of isStaticArray to document the current behaviour (see dlang/dlang.org#2507). But if this is intended behaviour it should be documented more obviously, especially because of the missmatch to is(...).

cc @WalterBright @ibuclaw @Geod24

@dlang-bot
Copy link
Contributor

dlang-bot commented Jan 23, 2021

Thanks for your pull request and interest in making D better, @MoonlightSentinel! 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
21570 normal __traits(isStaticArray, ...) accepts enums with static array as base type

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

@MoonlightSentinel MoonlightSentinel changed the title Fix 21570 - __traits(isStaticArray, ...) accepts enums with static... Issue 21570 - Deprecate __traits(isStaticArray, ...) accepts enums with static... Jan 23, 2021
@MoonlightSentinel MoonlightSentinel changed the title Issue 21570 - Deprecate __traits(isStaticArray, ...) accepts enums with static... Issue 21570 - Deprecate __traits(isStaticArray, ...) accepts enums... Jan 23, 2021
@RazvanN7
Copy link
Contributor

Why did you delete Fix keyword? This does bring a solution to the issue.

@MoonlightSentinel
Copy link
Contributor Author

Because this PR just adds a warning for the user, the current behaviour remains unchanged, But I can re-add the fix if preferred.

@RazvanN7
Copy link
Contributor

My point was that the deprecation should close the issue. An action was taken (i.e. the behavior is deprecated and will be changed) and the bug report no longer has any usefulness. So please re-add the "Fix" keyword.

@MoonlightSentinel MoonlightSentinel changed the title Issue 21570 - Deprecate __traits(isStaticArray, ...) accepts enums... Fix Issue 21570 - Deprecate __traits(isStaticArray, ...) accepts enums... Jan 25, 2021
@MoonlightSentinel
Copy link
Contributor Author

Fair enough

@RazvanN7
Copy link
Contributor

Can you also, please, add a changelog entry?

...with static array as base type.

The spec[1] states that a named enum creates a distinct type which is
implicitly convertible to it's base type. Hence `__traits(isStaticArray, <enum>)`
should yield `false` as done for an equivalent `is(...)` expression.

[1] https://dlang.org/spec/enum.html#named_enums
@MoonlightSentinel
Copy link
Contributor Author

Sorry, see static_array_enum.md

@RazvanN7
Copy link
Contributor

This would also require a spec change . Specifically, this: https://dlang.org/spec/traits.html#isStaticArray

@MoonlightSentinel MoonlightSentinel added Needs Approval Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org labels Jan 25, 2021
@RazvanN7
Copy link
Contributor

RazvanN7 commented Mar 5, 2021

After discussing with Walter, it seems that this is not actually a bug. __traits(isStaticArray) is designed to see through to the base type. Other traits such as isArithmetic, isFloating etc. work similarly (e.g. in the case of enums they use the basetype), therefore what should be done is to update the spec to clarify this.

@MoonlightSentinel MoonlightSentinel deleted the traits-staticarray-enum branch March 23, 2022 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Needs Approval Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org
Projects
None yet
3 participants