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

Explicitly defer node when attempting deferring an aliased modification in an imported macro function #1120

Merged
merged 2 commits into from
Oct 20, 2023

Conversation

jasmith-hs
Copy link
Contributor

Explicitly fail by creating a deferred node when attempting to defer modification of an alias outside of the context of an import.

This is a niche situation that we'd rather explicitly fail on than have incorrect logic. Handling this correctly is complex and likely not worth it.

Example:

{# 'eager/settings.jinja' #}
{% set settings = {} %}

{% macro load_settings() %}
{% do settings.put('foo', 'bar') %}
{% endmacro %}
{# 'main.jinja' #}
{% import 'eager/settings.jinja' as shared %}

{% if deferred %}
{{ shared.load_settings() }}
{% endif %}
{{ shared.settings }}

We don't know how that shared should be deferred so shared.settings ends up getting resolved as {}. This is what the output looks like before this PR:

{% if deferred %}
{% set __macro_load_settings_2043810590_temp_variable_0__ %}
{% set settings = {}  %}{% do settings.put('foo', 'bar') %}
{% endset %}{{ __macro_load_settings_2043810590_temp_variable_0__ }}
{% endif %}
{}

I tried actually supporting it, but since we can't reconstruct alias maps with macro functions inside of them outside of an import tag anyway, I decided to just short circuit the failure rather than adding a bunch of extra logic which ultimately flags the same failure condition. Here's that branch: ea4a7ac

…modification of an alias outside of the context of an import.

This is a niche situation that we'd rather explicitly fail on than have incorrect logic. Handling this correctly is complex and likely not worth it.

Example:
```
{# 'eager/settings.jinja' #}
{% set settings = {} %}

{% macro load_settings() %}
{% do settings.put('foo', 'bar') %}
{% endmacro %}
```

```
{# 'main.jinja' #}
{% import 'eager/settings.jinja' as shared %}

{% if deferred %}
{{ shared.load_settings() }}
{% endif %}
{{ shared.settings }}
```
Base automatically changed from sharing-alias-name to master October 11, 2023 20:20
@jasmith-hs jasmith-hs merged commit 23695ab into master Oct 20, 2023
4 checks passed
@jasmith-hs jasmith-hs deleted the aliased-macro-modification branch October 20, 2023 19:25
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.

1 participant