Skip to content

define available constants#6401

Merged
headius merged 2 commits intojruby:masterfrom
ahorek:available_constants
Sep 19, 2020
Merged

define available constants#6401
headius merged 2 commits intojruby:masterfrom
ahorek:available_constants

Conversation

@ahorek
Copy link
Contributor

@ahorek ahorek commented Sep 18, 2020

No description provided.

@ahorek ahorek changed the title WIP: define available constants define available constants Sep 18, 2020
@headius
Copy link
Member

headius commented Sep 19, 2020

I think this should probably check if the enum in question implements Constant before casting, and only use the defined logic in that case. It doesn't look like we use those methods with any non-Constant Enums but since we can't change the public API we should make it work in that case.

Otherwise it looks fine.

@headius headius added this to the JRuby 9.3.0.0 milestone Sep 19, 2020
@headius
Copy link
Member

headius commented Sep 19, 2020

I realize now that master already did the blind cast to Constant, and I have a different change in mind. This is fine to merge.

@headius headius merged commit f45b462 into jruby:master Sep 19, 2020
headius added a commit that referenced this pull request Sep 19, 2020
Previously this was a blind case, followed by silently rejecting
non-Constant enums. Better to prevent improper use at the type
level.

See #6401.
@headius
Copy link
Member

headius commented Sep 19, 2020

I decided to go ahead and fix the constant set logic to enforce the Constants interface at the generic level. Since this method never worked unless the enum in question implemented Constant, I think it's best to just make that restriction explicit.

headius added a commit to headius/jruby that referenced this pull request May 17, 2021
Previously this was a blind case, followed by silently rejecting
non-Constant enums. Better to prevent improper use at the type
level.

See jruby#6401.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants