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

Add defined? -> instance_variable_defined? mutation #1155

Merged
merged 3 commits into from
Dec 14, 2020

Conversation

dgollahon
Copy link
Collaborator

meta/defined.rb Outdated
singleton_mutations
mutation 'defined?(nil)'
mutation 'defined?(foo)'
mutation 'true'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would do this as a separate PR, but should the defined? mutator also emit false? It's interesting that it is only true. Is there a reason for that?

Copy link
Owner

Choose a reason for hiding this comment

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

I think this was a deliberate decision as singleton_mutations never emits false:

irb(main):001:0> defined?(@a)
=> nil
irb(main):002:0> @a = "foo"
=> "foo"
irb(main):003:0> defined?(@a)
=> "instance-variable"

emitting true is also wrong in that light ;) I'll add a commit to improve this.

Copy link
Collaborator Author

@dgollahon dgollahon Dec 14, 2020

Choose a reason for hiding this comment

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

Ah, I guess since we are emitting nil we already have something falsey.

@mbj I actually think true was a good mutation--don't we normally replace predicates with true and false? (or maybe there is an issue for this somewhere.

Or are you thinking it's just too hard to cover normal defined? mutations because it's hard to have both states?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh! nevermind. I see that defined? never returns true normally. Ok, cool, good call, ignore me. I misunderstood what you were saying for a moment. Nice improvement.

Copy link
Owner

Choose a reason for hiding this comment

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

no worries, defined? is weird in other ways.

It actually gives information about the AST, but when the AST references a constant, than it also checks the presence of that constant. Ruby....

Comment on lines +27 to +36

* Add subcommand `environment subject list`. It allows to list
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My editor trimmed some trailing whitespace. I can remove this from the commit if you want but also seems like it is worth fixing.

Copy link
Owner

Choose a reason for hiding this comment

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

Added an extra commit, and my editors whitespace trimming broke ;) Good catch.

- Mutates expressions where `defined?` is being used to check specifically for an instance variable such as `defined?(@A)` to the more narrow `instance_variable_defined?(:@A)` instead.
- Closes #1138.
@mbj mbj force-pushed the add-ivar-defined-mutation branch 3 times, most recently from 36228e9 to a9a303e Compare December 14, 2020 01:32
* `defined?` can never return `true` hence mutating to it as surrogate
  for truthy values does not make lots of sense. It would also assume
  callers exclusively use it as a predicate, which may not be true.
* Remove uncanonical defined? argument mutations.
@mbj mbj merged commit f5db70b into master Dec 14, 2020
@mbj mbj deleted the add-ivar-defined-mutation branch December 14, 2020 01:54
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.

Add mutation from defined(@ivar) to instance_variable_defined?(:ivar).
2 participants