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

Implementation: allow to lookup class and module implementations #7742

Conversation

@MakeNowJust
Copy link
Contributor

commented May 6, 2019

Fixed #4941

Image:

2019-05-06 8:39:32

I think it is totally useful, however this implementation cannot work on some corner case. One corner case, method argument restriction:

def foo(x : ‸Foo)
end

foo

It is hard to implement for now. Another corner case, macro argument:

record Foo,
  x : ‸Bar

It is impossible.


@faustinoaq Sorry for the late. Is this okay?

MakeNowJust added some commits May 5, 2019

@RX14

RX14 approved these changes May 7, 2019

@MakeNowJust

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

ping. Could anyone review this?

༓class Foo
end
alias Bar = Foo

This comment has been minimized.

Copy link
@bcardiff

bcardiff May 11, 2019

Member

Was there an explicit decision to not resolve to this line instead the Foo? What happens if the alias has a union?

MakeNowJust added some commits May 13, 2019

@MakeNowJust

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

@bcardiff Thank you for reviewing!

I fixed crystal tool implementation to lookup alias name without resolve. For example:

foo.cr:

class Foo
end

class Bar
end

alias Baz = Foo | Bar

Baz
$ bin/crystal tool implementation -c foo.cr:9:1 foo.cr
1 implementation found
foo.cr:7:1

Unfortunately this doesn't work:

$ bin/crystal tool implementation -c foo.cr:7:15 foo.cr
no implementations or method call found

I think implementing this is hard for now...

@sdogruyol
Copy link
Member

left a comment

Thank you @MakeNowJust 👍

@bcardiff
Copy link
Member

left a comment

I was a bit worried about the introduction of target_type in Path it's actually similar to target_const.

Please, create an issue to track the left-over for the lookup from the alias definition.

Thanks!

@bcardiff bcardiff added this to the 0.29.0 milestone May 13, 2019

@faustinoaq

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Hi @MakeNowJust Yeah, this is a progress, Thank you! 👍

@bcardiff bcardiff merged commit aaf05d9 into crystal-lang:master May 15, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MakeNowJust MakeNowJust deleted the MakeNowJust:fix/4941-crystal-tool-implementation-class branch May 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.