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

Isolate included macro evaluation context from including type #9545

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

makenowjust
Copy link
Contributor

Fixed #9488

Now included macro is evaluated on included module context. It makes intuitive and useful result in many cases. And, extended macro is also evaluated on extended module context.

One example is #9488, and other example is the following:

module Collect
  NAMES = [] of String

  macro included
    {% NAMES << @type.name.stringify %}
  end
end

class Foo
  include Collect
end

class Bar
  NAMES = ["hey"]
  include Collect
end

p Collect::NAMES

The last p prints ["Foo"] currently, however my expected result is ["Foo", "Bar"]. This PR also fix such case.

MEMO: it is breaking change.

Fixed crystal-lang#9488

Now `included` macro is evaluated on included module context.
It makes intuitive and useful result in many cases.
And, `extended` macro is also evaluated on extended module context.
@@ -992,21 +992,21 @@ class Crystal::TopLevelVisitor < Crystal::SemanticVisitor

begin
current_type.as(ModuleType).include type
run_hooks hook_type(type), current_type, kind, node
run_hooks hook_type(type), current_type, kind, node, path_lookup: type
Copy link
Member

@asterite asterite Jun 24, 2020

Choose a reason for hiding this comment

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

Shouldn't this be path_lookup: current_type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. It will not change anything.

end

Bar.foo
") { int32 }
Copy link
Member

Choose a reason for hiding this comment

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

I think should be float64. The type should be looked up in the including type. That's why we should use current_type.

Note that the hook is already executed in the including type, for example if you add a method to it. So type lookup should also be done from the including type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, the hook name is just included. So, it seems intuitive that included module executes it.

In fact, Ruby's included hook works in such way:

module Foo
  A = :Foo

  def self.included(klass)
    p A
  end
end

class Bar
  A = :Bar
  include Foo
end

# output:
# :Foo

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but in Ruby you get the klass as the including type, we don't do that in Crystal because we don't have runtime define_method and other stuff.

As I said, the current semantic should be revised before we try to change it or fix/brake things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #5354. I consider included hook is a feature that it appends Foo.included automatically at include Foo if Foo has included macro, so this fix is correct in this view.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure #5354 is correct. It's unintuitive that the macro looks up type in the call scope and not the scope where the macro is defined.

All of this to say: we should formalize the semantics that we want here and then implement it. Right now this was all done with guesses and heuristics. This PR might fix something for some, I'm sure it will break code for others and we can't do that right before 1.0.0, specially not without understanding what we really want.

@asterite
Copy link
Member

We shouldn't merge this PR until we decide what semantic we want for this. There are conflicting interests.

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.

Shadowed type var names break resolution in macros
2 participants