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

[2.0.10 Regression] Methods defined in a template can't be used later in the template #347

Open
ldionne opened this issue Sep 24, 2019 · 19 comments

Comments

@ldionne
Copy link

commented Sep 24, 2019

Starting with Tilt 2.0.10, methods defined in a template can't be used later in the template, it seems. Here's a self-contained reproducer that works with Tilt 2.0.9, but not Tilt 2.0.10:

$ cat <<EOF > repro.erb.txt
<%
  def foo(x)
    puts x
  end
%>
<%= foo('hello') %>
EOF

$ ruby -r tilt -e "Tilt::ERBTemplate.new('repro.erb.txt').render"

The output with Tilt 2.0.9 is hello, but the output with Tilt 2.0.10 is:

Traceback (most recent call last):
    4: from -e:1:in `<main>'
    3: from <...>/tilt-2.0.10/lib/tilt/template.rb:109:in `render'
    2: from <...>/tilt-2.0.10/lib/tilt/template.rb:170:in `evaluate'
    1: from <...>/tilt-2.0.10/lib/tilt/template.rb:170:in `call'
repro.erb.txt:6:in `__tilt_70208412901580': undefined method `foo' for #<Object:0x00007fb546938d40> (NoMethodError)
@ldionne

This comment has been minimized.

Copy link
Author

commented Sep 24, 2019

Looking at the diff between 2.0.9 and 2.0.10 here, I would be tempted to say that the culprit is be625c8.

@pdadhaniya

This comment has been minimized.

Copy link

commented Sep 24, 2019

Just experienced this issue as well after upgrading

@judofyr

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2019

Yikes! Can you look into this @jeremyevans?

@judofyr

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2019

https://github.com/hanami/hanami/issues/1015 seems to be a separate bug related to class scoping:

Boot Error

Something went wrong while loading .../project/config.ru
NameError: uninitialized constant Hanami::View::Rendering::Scope::String Did you mean? String StringIO STDIN

.../project/apps/web/templates/account/login.html.erb:1:in `__tilt_47450820803780'
.../.rbenv/versions/2.4.3/lib/ruby/gems/2.4.0/gems/tilt-2.0.10/lib/tilt/template.rb:170:in `call'
.../.rbenv/versions/2.4.3/lib/ruby/gems/2.4.0/gems/tilt-2.0.10/lib/tilt/template.rb:170:in `evaluate'
.../.rbenv/versions/2.4.3/lib/ruby/gems/2.4.0/gems/tilt-2.0.10/lib/tilt/template.rb:109:in `render'
@jeremyevans

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

Taking a look now. I'm guessing the method issue is because we are not using the singleton class. Not sure about the hanami scope issue at this point.

@jeremyevans

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

The issue with the method definition is because in 2.0.10, foo is defined in Tilt::TOPOBJECT (which is Tilt::CompiledTemplates in Ruby >2.0). To fix this and still work on Ruby 2.7 without warnings, we would have to add a way to provide a replacement for Tilt::TOPOBJECT that could be specified by the user. This couldn't be the scope_class by default, because that would break cases where scope_class is frozen.

Note that a simple workaround for this issue is to use def self.foo instead of def foo. Applications defining methods with def foo were relying on undefined behavior.

@jeremyevans

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

The hanami issue I'm not sure about, I would need the content of the template that is failing, the scope it is being executed on, and which constants are defined (or even better, a reproducible example).

@jeremyevans

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

These issues show that the new behavior is not backwards compatible in all cases and probably cannot be made backwards compatible in such cases. However, the new behavior is necessary to continue to work on newer versions of Ruby, and offers significant performance benefits on all Ruby versions. Therefore, I propose we make it opt-in in older ruby versions, and required in ruby 2.7+. Alternatively, we release 2.0.11 with the changes reverted, and release 3.0.0 with the changes added back.

@jodosha

This comment has been minimized.

Copy link

commented Sep 24, 2019

hanami-view rendering scope (Hanami::View::Rendering::LayoutScope) inherits from BasicObject.

By temporary removing this inheritance the broken tests passes again:

diff --git a/lib/hanami/view/rendering/layout_scope.rb b/lib/hanami/view/rendering/layout_scope.rb
index 5c5fca3..a6ee651 100644
--- a/lib/hanami/view/rendering/layout_scope.rb
+++ b/lib/hanami/view/rendering/layout_scope.rb
@@ -16,7 +16,7 @@ module Hanami
       # Scope for layout rendering
       #
       # @since 0.1.0
-      class LayoutScope < BasicObject
+      class LayoutScope
         # Initialize the scope
         #
         # @param layout [Hanami::Layout] the layout to render

I cannot apply this patch above, because BasicObject causes less method name clashes than Object.

For instance, the HTML helper select (to generate <select> tag), clashes with Kernel#select, which is inherited by the descendants of Object, but not BasicObject. See hanami/view#28

@jodosha

This comment has been minimized.

Copy link

commented Sep 24, 2019

By debugging be625c8#diff-4757dfc1f8a5b4b4ab833adbe73c5785R169 I found an interesting, but confusing detail:

When Hanami::View::Rendering::LayoutScope inherits from BasicObject, scope.is_a?(Module) returns true:

(byebug) scope
#<Hanami::View::Rendering::LayoutScope:7fbc05b25d90 @layout="#<LocalsLayout:0x00007fbc05ba5a40 @scope=#<Hanami::View::Rendering::LayoutScope:7fbc05b25d90 @layout="#<LocalsLayout:0x00007fbc05ba5a40 ...>" @scope="#<Hanami::View::Rendering::Scope: 7fbc05b25d90 @view="Hanami::View::Rendering::NullView" @locals="{:format=>:html}">">, @rendered="">" @scope="#<Hanami::View::Rendering::Scope: 7fbc05b25d90 @view="Hanami::View::Rendering::NullView" @locals="{:format=>:html}">">
(byebug) scope.is_a?(Module)
true

When it implicitly inherits from Object, scope.is_a?(Module) returns false:

(byebug) scope
#<Hanami::View::Rendering::LayoutScope:7fdc67add928 @layout="#<LocalsLayout:0x00007fdc67addc20 @scope=#<Hanami::View::Rendering::LayoutScope:7fdc67add928 @layout="#<LocalsLayout:0x00007fdc67addc20 ...>" @scope="#<Hanami::View::Rendering::Scope: 7fdc67add9f0 @view="Hanami::View::Rendering::NullView" @locals="{:format=>:html}">">, @rendered="">" @scope="#<Hanami::View::Rendering::Scope: 7fdc67add9f0 @view="Hanami::View::Rendering::NullView" @locals="{:format=>:html}">">
(byebug) scope.is_a?(Module)
false

With the default scenario (BasicObject subclass), the ternary operator returns the first branch, but it should return the second instead. @jeremyevans please correct me if I'm wrong.

method = compiled_method(locals_keys, scope.is_a?(Module) ? scope : scope.class)

I'm using this Ruby version:

$ ruby --version
ruby 2.6.3p62 (2019-04-16 revision 67580) [x86_64-darwin16]
@jeremyevans

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

That makes sense. String is not in BasicObject constant lookup scope. You would need to use ::String in the template. I'd say this is another case that relied on undefined behavior. This worked previously because the scope during lookup was ultimately Object as the string of code was passed to Object.class_eval (now it is passed to scope_class.class_eval).

Alternatively, you could fix constant lookup for your BasicObject subclass:

class LayoutScope < BasicObject
    def self.const_missing(name)
      ::Object.const_get(name)
    end
end
@jeremyevans

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

Actually, if you pass a BasicObject/LayoutScope instance as a scope, the code should fail with a NoMethodError (unless you are also defining an is_a? method on the class. That is fixable in tilt:

diff --git a/lib/tilt/template.rb b/lib/tilt/template.rb
index 7f35cb2..9accf46 100644
--- a/lib/tilt/template.rb
+++ b/lib/tilt/template.rb
@@ -166,7 +166,12 @@ module Tilt
     def evaluate(scope, locals, &block)
       locals_keys = locals.keys
       locals_keys.sort!{|x, y| x.to_s <=> y.to_s}
-      method = compiled_method(locals_keys, scope.is_a?(Module) ? scope : scope.class)
+      case scope
+      when BasicObject
+        method = compiled_method(locals_keys, Kernel.instance_method(:class).bind(scope).call)
+      else
+        method = compiled_method(locals_keys, Module === scope ? scope : scope.class)
+      end
       method.bind(scope).call(locals, &block)
     end

However, then if you are using Erubi as the erb processor, it fails as well, because Erubi uses String and not ::String as the output buffer. I can fix that in erubi. That may be the bug actually hit by Hanami.

ldionne added a commit to ldionne/hana that referenced this issue Sep 24, 2019
See rtomayko/tilt#347 for details.
@ldionne

This comment has been minimized.

Copy link
Author

commented Sep 24, 2019

FWIW, I'm personally fine with the suggested workaround of using def self.foo.

@jeremyevans

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

I committed a fix to Erubi: jeremyevans/erubi@3db54fc

@jodosha, can you try the Erubi master branch and see if that fixes the Hanami issue?

@jodosha

This comment has been minimized.

Copy link

commented Sep 25, 2019

@jeremyevans thanks for the quick patch on erubi. However, the problem isn't just for ::String. For instance, while running hanami-view specs with tilt 2.0.10, I get a similar error but for Haml:

Failures:
  1) Hanami::View rendering uses HAML engine
     Failure/Error: %h1= person.name
     
     NameError:
       uninitialized constant Hanami::View::Rendering::Scope::Haml
     # ./spec/support/fixtures/templates/contacts/show.html.haml:1:in `__tilt_14195100'
     # ./vendor/bundle/ruby/2.6.0/gems/tilt-2.0.10/lib/tilt/template.rb:170:in `call'
     # ./vendor/bundle/ruby/2.6.0/gems/tilt-2.0.10/lib/tilt/template.rb:170:in `evaluate'
     # ./vendor/bundle/ruby/2.6.0/gems/tilt-2.0.10/lib/tilt/haml.rb:24:in `evaluate'
     # ./vendor/bundle/ruby/2.6.0/gems/tilt-2.0.10/lib/tilt/template.rb:109:in `render'
     # ./lib/hanami/view/template.rb:47:in `render'
     # ./lib/hanami/view/rendering.rb:140:in `rendered'
     # ./lib/hanami/view/rendering.rb:154:in `layout'
     # ./lib/hanami/view/rendering.rb:108:in `render'
     # ./lib/hanami/view/rendering.rb:259:in `render'
     # ./spec/unit/hanami/view_spec.rb:286:in `block (3 levels) in <top (required)>'

See https://travis-ci.org/hanami/view/jobs/589128538

@judofyr

This comment has been minimized.

Copy link
Collaborator

commented Sep 25, 2019

I do believe that we actually intentionally said that constant lookup should be based in Object even though the scope is based on BasicObject. Doesn't seem to be a test for it though…

@jodosha

This comment has been minimized.

Copy link

commented Sep 25, 2019

@jeremyevans You hit the point:

NoMethodError (unless you are also defining an is_a? method on the class.)

Hanami::View::Rendering::LayoutScope defines #method_missing to forward message passing to the scope that it wraps. Because of this setup, #is_a? is dynamically delegated to @scope. This implicit behavior, can be the cause of the unexpected returned value for is_a?(Module) (see #347 (comment)).


I cannot recall why Hanami::View::Rendering::LayoutScope inherits from Ruby's BasicObject, rather than from Hanami::Utils::BasicObject (from hanami-utils gem), which properly handles #is_a?.

By implementing Hanami::Utils::BasicObject.const_missing (as suggested by @jeremyevans) and making Hanami::View::Rendering::LayoutScope to inherit from Hanami::Utils::BasicObject, the build for hanami-view passes when using tilt 2.0.10.


The problem is solved for Hanami. Can I do something to help with tilt?

@jeremyevans

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

@judofyr Unfortunately, you can't have constant lookup fallback to Object and have constant lookup based on the scope class and run without warnings on Ruby 2.7. If the scope class is subclassed from BasicObject, and you want to reference constants in Object without :: preceding them, you need to implement const_missing in the scope class and delegate constant lookup to Object.

I'll push out a new release of Erubi today.

@jeremyevans

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

Erubi 1.9.0 was released a few minutes ago. I'll work on a pull request to fix the rendering of BasicObject instances in tilt.

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