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

Fixed optimization of filters and misoptimizations with changed eval context #476

Closed
wants to merge 3 commits into from
Closed

Conversation

snoack
Copy link
Contributor

@snoack snoack commented Aug 8, 2015

I realized that some filters aren't inlined during optimizations. It turned out that this effects all environment and eval context filters. That is because the environment or eval context isn't passed as first but second argument. Therefore the filter function raises an exception and the optimizer skips the node. This one was easy to fix, see the first commit in this PR.

However, now things get funny. With that fix, the tests for the {% autoescape %} tag suddenly start to fail. As it turned out the optimizer were agnostic to the eval context, and the as_const() method created implicitly a new eval context for every node, ignoring {% autoescape %} tags up the tree. So I fixed this as well, see the second commit in this PR. Therefore optimizer now keeps track of the eval context, and updates it when visiting ScopedEvalContextModifier nodes. To avoid future issues like that and to get rid of dead code, I also made the eval context passed to as_const() mandatory.

@jeffwidman
Copy link
Contributor

jeffwidman commented Apr 15, 2016

I suspect you haven't gotten much feedback simply because there's a lot going on here. I don't have the time to review it in depth either right now.

Since none of these change external APIs (other than fixing bugs), then if you are confident that these are still good, I'd say go ahead and merge it, along with #478 and #479 since they all seem to be related.

@davidism
Copy link
Member

davidism commented Jul 5, 2017

I'm having trouble merging and evaluating this. The first commit is already applied in master, and no tests fail, or appear to have been changed along with that commit. While keeping the context during optimization seems correct, I don't know the original intention for not doing this. I cannot get Optimizer.visit_EvalContextModifier to trigger, so I don't know if this is needed. The final commit also does not appear to be necessary.

@snoack if you can submit a new PR that merges and has tests, that would be appreciated.

@davidism davidism closed this Jul 5, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants