tdammers / ginger

A Haskell implementation of the Jinja template language.
MIT License
77 stars 13 forks source link

don't ignore non-existent variables #61

Open plsnp opened 4 years ago

plsnp commented 4 years ago

Right now when printing {{doesnt_exist}} it just doesn't print anything. Rather i would like to have throw an exception or even better check on compile time. Now bugs are there and they are not notified about.

tdammers commented 4 years ago

This is unfortunately an unwinnable war.

While it would be nice to make nonexistent variables errors, it would also require passing Nothings for every required value that we want to not pass; and the problem with that is that templates are provided at runtime, so we cannot tell at compile time which values will be required.

Also, a rather common approach in dynamic HTML templating is to have templates with lots of optional arguments, allowing the same template to be reused across many different concrete data types. This would then require the host environment to provide a giant list of Nothing values, or the template itself would have to do a definedness test for each variable it wants to read. And because Ginger is not intended to be used as a general-purpose programming or scripting language (though if you really really want to, it can be that), but a templating language, avoiding boilerplate in templates is absolutely paramount, and those definedness checks would ruin that.

I do understand the concern though, and have, in fact, been bitten by it myself a couple times. So I guess if you were to come up with a patch that offers warnings about undefined variables as an opt-in flag, I'd be game.

Compile-time checks however are going to be a whole lot more complex than you'd initially think, because they would basically boil down to adding a type checker, and to make it completely reliable (and thus useful for hard errors), the type system would have to support dependent types, because we can do things like dynamically construct strings and use those to look up variables at runtime, and this in turn means that the type system needs to be able to express things like "a dictionary that has a key for every string that this function here can output given an input of type X". This is not a project for an afternoon.

On this note: ginger sits in a bit of a niche - unlike most other Haskell HTML templating solutions, it parses and interprets templates at runtime. If you don't need this, I would recommend using something else instead - Blaze, for example, is fantastic, and leverages the full glory of Haskell's type system.

plsnp commented 4 years ago

How does the runtime resolvement of variable name to underlying value work? Where is the code in the ginger codebase for this? Could it not just be added that when the templated "requests a value" which is not present an exception is thrown?

Also, a rather common approach in dynamic HTML templating is to have templates with lots of optional arguments, allowing the same template to be reused across many different concrete data types.

I've only seen templates that do something like

{% if obj is defined%}{{obj.hello}}{% endif %}

Optional arguments without defined checks are poorly written templates IMO and this use case should not be supported.

or the template itself would have to do a definedness test for each variable it wants to read.

Don't really understand this. Ginger should check if the variable is in the hashmap yes, but the template can do 1 check for more variables.

{% if obj is defined%}
   {{obj.hello}}
   {{obj.world}}
{% endif %}

Did you look how jinja2 in python handles this case? I know for sure that PHP's twig throws exception when the template requests a value which is not present. I understand the argument for compile time being difficult, but why should PHP be in the dynamic case be "more powerful" than a haskell implementation?

tdammers commented 3 years ago

Optional arguments without defined checks are poorly written templates IMO and this use case should not be supported.

That's a strong opinion, and I beg to differ. But that's beside the point, because I already said that if you come up with a patch, I'm up for it.

Don't really understand this. Ginger should check if the variable is in the hashmap yes, but the template can do 1 check for more variables.

OK, let me try to explain this from a different angle.

First of all, there is no hashmap. Variable lookups ultimately bubble up to contextLookup, defined in the Text.Ginger.Run.Type module. If you want to throw an error or print a warning from there, you can; you don't need to change a line of Ginger source code for that, you just need to use a different contextLookup than the one easyContext provides out of the box, or use the "fancy" API (makeContext...), which allows you to specify a custom context lookup function, from which you can warn and throw to your heart's desire.

The bigger issue is writing defined, and specifically, that it cannot possibly be written as a function, unless we descend to JavaScript levels of lunacy and introduce undefined as a "value, only not really". The way the engine works right now, in order to call defined as a function, we would first need a successful lookup for its argument - but the whole point of defined is to check whether such a lookup would succeed if we attempted it.

But if defined cannot be written as a function, then as what? There is no other Ginger language construct that could do it (remember, filters and tests are just syntax sugar over functions), so we would have to either special-case defined as a keyword, or add an entire class of language constructs (macros?) just for this one thing. Keep in mind that because defined looks like a filter (and thus function), it should behave like one under all reasonable circumstances, so things like {% set x = defined(foo[bar]) %} should also work, so a simple syntax-level special case, like what for example Selmer does, won't fly). I'm not quite sure at all what a good way of implementing this would be. The best I can come up with is this:

  1. Add a DefinedE constructor to the Expr data type.
  2. Special-case defined in the parser to parse into DefinedE Expr instead of VarE "defined".
  3. In the interpreter, when encountering a DefinedE, implement definedness checks for the subexpression in the current context.

That last one is of course the tricky bit: it's easy for DefinedE (VarE ...), but for other expressions, it gets interesting fast.

Actually, you know what, I might give it a shot, because why not.