twigcs icon indicating copy to clipboard operation
twigcs copied to clipboard

Macros defined in outside scope are not considered used by `UnusedMacro` when used inside a macro

Open sustmi opened this issue 3 years ago • 1 comments

Issue https://github.com/friendsoftwig/twigcs/issues/27 says:

The body of a macro is a fully isolated scope. So an import done inside a macro must be checked for usage only inside the macro. And an import done outside the macro must look for usages outside the macro too (finding a usage inside the macro is not valid, as it cannot reference the macro).

However, the current Twig documentation says:

Imported macros are always local to the current template. It means that macros are available in all blocks and other macros defined in the current template, but they are not available in included templates or child templates; you need to explicitly re-import macros in each template.

https://twig.symfony.com/doc/3.x/tags/macro.html#macros-scoping

This means that the following code is valid use of foo macro:

{% import "foo.html.twig" as foo %}
{% macro bar() %}
    {{ foo.stuff() }}
{% endmacro %}

However, the UnusedMacro thinks that foo macro is not used and issues the following violation:

l.1 c.29 : ERROR Unused macro import "foo".

Additional notes:

I found this comment in the Scope class:

/**
 * When isolated, a scope won't be explored when looking for name usages.
 */
public function isolate()
{
    $this->isolated = true;
}

https://github.com/friendsoftwig/twigcs/blob/2634cc261c89e558a1b8198daf579414357ec8ed/src/Scope/Scope.php#L53

However this should not be true when looking for usages of macros.

Steps to reproduce

I created a failing test case (that should be passing) here: https://github.com/sustmi/twigcs/blob/6965bad2494b811c54030eed8a7b3682beb8851c/tests/Twig3FunctionalTest.php#L258

  1. git clone https://github.com/sustmi/twigcs/
  2. cd twigcs
  3. git checkout unused-macro-issue-180
  4. composer install
  5. phpunit tests/Twig3FunctionalTest.php

sustmi avatar Jul 02 '21 17:07 sustmi

There is one noteworthy behavior.

One would think that in this code:

{% block body %}
    {% import "foo.html.twig" as foo %}
    {% macro bar() %}
        {{ foo.stuff() }}
    {% endmacro %}

    {{ _self.bar() }}
{% endblock %} 

the foo macro would be accessible from bar macro context, but Twig runtime says that foo macro does not exist.

I think that the devil lies in detail. The Twig documentation says (emphasis is mine):

When calling import or from from a block tag, the imported macros are only defined in the current block and they override macros defined at the template level with the same names.

This probably means that macro imported inside block tag is only defined in the block itself and not in sub-scopes. Or in another words: the visibility of imported macro in block or macro tags depends on whether the import is done in top-level scope or inside block scope. Not very intuitive I would say.

If I import the macro outside the body block, the code works:

{% import "foo.html.twig" as foo %}

{% block body %}
    {% macro bar() %}
        {{ foo.stuff() }}
    {% endmacro %}

    {{ _self.bar() }}
{% endblock %} 

However, that does not explain why this code works:

{% block body %}
    {% import "foo.html.twig" as foo %}
    {% set arr = [1,2] %}
    {% for item in arr %}
        {{ foo.stuff() }}
    {% endfor %}
{% endblock %}

because for loop also has its own scope (see the closing note here: https://twig.symfony.com/doc/3.x/tags/set.html) but the foo macro is accessible in this case.

I guess that for scope is not the same as macro or block scope because when I substitute the for loop with block, this code does not work:

{% block body %}
    {% import "foo.html.twig" as foo %}
    {% block bar %}
        {{ foo.stuff() }}
    {% endblock %}
{% endblock %} 

while the following code works (ie. the behavior is the same as with macro tag inside block):

{% import "foo.html.twig" as foo %}

{% block body %}
    {% block bar %}
        {{ foo.stuff() }}
    {% endblock %}
{% endblock %} 

sustmi avatar Jul 02 '21 18:07 sustmi