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 TypeNode#has_method? #4474

Merged
merged 2 commits into from Jun 8, 2017
Merged

Add TypeNode#has_method? #4474

merged 2 commits into from Jun 8, 2017

Conversation

Sija
Copy link
Contributor

@Sija Sija commented May 27, 2017

Fixes #4472

@bew
Copy link
Contributor

bew commented May 27, 2017

Nice addition, you should add some specs for this

@Sija
Copy link
Contributor Author

Sija commented May 28, 2017

@bew there aren't any for the rest of related methods, I can add it though.

@bcardiff
Copy link
Member

@Sija yes, please let's add some spec in /spec/compiler/codegen/macro_spec.cr.

I would suggest also to split the fixes in the docs in other PR 🙏

Regarding the functionality, I have some doubts how it should interact with method_missing. Currently responds_to returns false if no method with that name is invoked somewhere, which is reasonable given that the types are fixed after the compilation finishes. But during macros evaluation, depending the evaluation order the has_method might return true if later a method_missing generates that method.

@@ -1533,6 +1538,15 @@ module Crystal
ArrayLiteral.new(defs)
end

def self.has_method?(type, name)
type.defs.try &.each do |name, metadatas|
Copy link
Member

Choose a reason for hiding this comment

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

You can simply use the name variable here, there's no need to traverse all metadatas

Copy link
Member

Choose a reason for hiding this comment

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

@Sija Ping :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BoolLiteral.new !!type.defs[name]? will do?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, or type.defs.has_key?(name)

Copy link
Member

Choose a reason for hiding this comment

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

Actually... type.has_def?(name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done :)

@Sija Sija force-pushed the fix-4472 branch 2 times, most recently from cde2491 to f5cec6b Compare June 7, 2017 14:14
@@ -1533,6 +1538,10 @@ module Crystal
ArrayLiteral.new(defs)
end

def self.has_method?(type, name)
BoolLiteral.new(!!type.has_def?(name))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that BoolLiteral.new(type.has_def?(name)) is enough, as Type#has_def? is already returning a boolean

Copy link
Contributor Author

@Sija Sija Jun 7, 2017

Choose a reason for hiding this comment

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

@bew nope, returned type is Bool | Nil. I'd say it should indeed return just Bool, yet it might be out of the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

right, sorry about that (found why: some_hash.try &.has_key?(some_key) can be Nil with the try..)

@Sija
Copy link
Contributor Author

Sija commented Jun 7, 2017

@bcardiff I've added specs as per request. I'm not sure what to do regarding method_missing and macros evaluation order though... any ideas? /cc @asterite

@asterite
Copy link
Member

asterite commented Jun 7, 2017

@Sija don't worry about that, it's a separate issue

@Sija
Copy link
Contributor Author

Sija commented Jun 7, 2017

Is it GTG? Travis CI xcode8.2 build failed, rest 's good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants