Skip to content

Turn class variable warnings into exceptions #2987

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

Merged

Conversation

jeremyevans
Copy link
Contributor

This changes the following warnings:

  • warning: class variable access from toplevel
  • warning: class variable @foo of D is overtaken by C

into RuntimeErrors. Handle defined?(@@foo) at toplevel
by returning nil instead of raising an exception (the previous
behavior warned before returning nil when defined? was used).

Refactor the specs to avoid the warnings even in older versions.
The specs were checking for the warnings, but the purpose of
the related specs as evidenced from their description is to
test for behavior, not for warnings.

Fixes [Bug #14541]

This changes the following warnings:

* warning: class variable access from toplevel
* warning: class variable @foo of D is overtaken by C

into RuntimeErrors.  Handle defined?(@@foo) at toplevel
by returning nil instead of raising an exception (the previous
behavior warned before returning nil when defined? was used).

Refactor the specs to avoid the warnings even in older versions.
The specs were checking for the warnings, but the purpose of
the related specs as evidenced from their description is to
test for behavior, not for warnings.

Fixes [Bug #14541]
Copy link
Member

@eregon eregon 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, thank you for doing this!
I forgot about this issue.

It would be useful to add new specs for the changed behavior, so other implementations also notice the semantics change and follow it.

@jeremyevans jeremyevans merged commit 900e83b into ruby:master Apr 10, 2020
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