fix chameleon default handling

[d-maurer]

Use the default_marker integration parameter rather than a locally defined marker("default") as representation of the TALES default object.

This fix would resolve "zopefoundation/Zope#853".

[malthe]

I need to integrate Travis in this repository, but I can see that this pull request introduces test failures: https://travis-ci.org/github/malthe/chameleon/builds/700298576.

Let me try and take a look at the Zope test failures.

[d-maurer]

Malthe Borch wrote at 2020-6-20 00:35 -0700:

I need to integrate Travis in this repository, but I can see that this pull request introduces test failures: https://travis-ci.org/github/malthe/chameleon/builds/700298576.

The reason is that the test apparently uses `literal_false == False`. In this case, "default_marker" is defined as `Buildin("False")`, i.a. "False" is interpreted as the representation of the default value.

[d-maurer]

Dieter Maurer wrote at 2020-6-20 10:44 +0200:

Malthe Borch wrote at 2020-6-20 00:35 -0700: >I need to integrate Travis in this repository, but I can see that this pull request introduces test failures: https://travis-ci.org/github/malthe/chameleon/builds/700298576. The reason is that the test apparently uses `literal_false == False`. In this case, "default_marker" is defined as `Buildin("False")`, i.a. "False" is interpreted as the representation of the default value.

The is a relevant code extract: ... # <Builtin id='False' ctx=<_ast.Load object at 0x7ff1159729d0> at 7ff11523eb10> -> __value __value = False __condition = (__expression is __value) if __condition: pass else: ... What is the use case for "literal_false == False"?

[d-maurer]

The PageTemplate parameter literal_false is documented as:

      ``literal_false``

        Attributes are not dropped for a value of ``False``. Instead,
        the value is coerced to a string.

        This setting exists to provide compatibility with the
        reference implementation.

It applies only to attributes and has nothing to do with the TALES default; instead it is strongly related to Python's False. (Likely, it should apply to all Python false values not just False.)

The documentation of MacroProgram.default_marker

    # This default marker value has the semantics that if an
    # expression evaluates to that value, the expression default value
    # is returned. For an attribute, if there is no default, this
    # means that the attribute is dropped.

seems to indicate that it implements the TALES default.

BUT, default_marker is defined as Builtin('__default') or Builtin('False') depending on literal_false. This looks wrong: the TALES default should be independent of literal_false.

[d-maurer]

I have changed the "literal_default" handling. The chameleon tests are now passing.

I have slightly changed 2 tests (more precisely 1 test to fix problems with 2 subtests). First I defined default to fix a "NameError: default not defined". After this the alternative default | ... no longer gave the second alternative (as the test required); I replaced default by undefined.
There is a further change in the tests -- but this one not voluntary: my editor could not handle some characters and replaced them by ?.

[malthe]

I think the default_marker naming here is a little confusing and misleading.

There are really two separate concerns here:

1. To enable the HTML-like "literal attributes" such as checked="checked" – where I believe there can be a default value, too – which is then either False or the string given in a static attribute definition. If literal attributes are not used, then we just use a regular marker object.
2. For attribute content, the default value should be the precomputed contents. But I think that for some reason, a marker value was used. This is probably a mistake.

The compiler definitely evolved organically and this shows in the design. Some of the abstractions seem quite leaky. The question is how much time should be invested in fixing it?

[d-maurer]

Malthe Borch wrote at 2020-6-21 01:25 -0700:

... The compiler definitely evolved organically and this shows in the design. Some of the abstractions seem quite leaky. The question is how much time should be invested in fixing it?

I think I detected the following design flaw. The expression engine has parameters "default" and "default_marker" with the following semantics: if an initial expression evaluation produces `default_marker`, the value is changed to `default`. The template machinery uses this feature to implement both `literal_false=False` (to special handle `False`) and the TAL/TALES `default` semantics. This works well in the simple case "`literal_false` and a single attribute". It cannot work with "not `literal_false`" -- then we need to special case both `False` and `default` -- nor with "dict attributes" (where we do not have a single attribute with a single default but several attributes with individual defaults). I think that the TAL engine and the expression engine should be disentangled. The expression engine should return the primary expression value. How to interpret this value (handling `default`, `False`, a dict for "dict" attributes) is a primary task of the TAL engine, not of the expression engine. I hope that it would not be difficult to implement this model. Idea: * the evaluation of dynamic attributes creates a list of name/value pairs (rather than sevaral variables `attr_<name> = value`). * in the result "dict attributes" are flattened out; i.e. a result element representing "dict attributes" are replaced by the list describing the individual attributes. * duplicates are removed from the list (or flagged as errors) * the dynamic attribute result is combined with the static attributes, handling `False` (if required), `default`, "boolean attributes", `None` appropriately. The result could even be significantly simpler than the current implementation (I was very impressed by e.g. the filtering related code (--> "what the hell is going on here") which I have not yet really understood). The implementation would be a bit less efficient because it does the combination of static and dynamic values at runtime while the current implemntation does it at compile time. Runtime combination is necessary for "dict attributes" because for this case, the defined attributes are not known at compile time. If necessary, the (likely rare) case "with dict attributes" could be handled specially.

[malthe]

The TALES engine just has to assign a value to the provided target.

I'm not sure I agree that it needs disentanglement from the TAL layer – which is currently implemented at the program level.

I think we're relatively close to a solution for the issue at hand here. Rewriting the whole handling of attributes is out of scope I think. There are a lot of corner cases – beware of dragons!

[d-maurer]

Malthe Borch wrote at 2020-6-22 15:51 +0000:

The TALES engine just has to assign a value to the provided target.

Why not.

I'm not sure I agree that it needs disentanglement from the TAL layer – which is currently implemented at the program level.

I tried to explain: * `not literal_false` requires special handling for both `False` and `default`. The TALES engine can handle at most one value * "dict attributes" require handling of `False` and `default` for each individual attribute - the TALES engine can only handle a single value.

I think we're relatively close to a solution for the issue at hand here. Rewriting the whole handling of attributes is out of scope I think. There are a lot of corner cases – beware of dragons!

It is your project. I will be satisfied if a decent integration into Zope (with "zope.tales" as TALES engine) is possible.

[malthe]

@d-maurer would you mind looking at #317 to see if this could fix the situation?

[d-maurer]

Malthe Borch wrote at 2020-6-22 11:06 -0700:

@d-maurer would you mind looking at #317 to see if this could fix the situation?

Unfortunately, it does not work. I wrote earlier that the default marker could be implemented in any way you like. Unfortunately, I was wrong. In the Zope world, the default expression type is "path" and it has the peculiarity that it checks if the located object is callable and if so calls it. You have implemented your `DEFAULT_MARKER` as a class and it is therefore callable. As a result, if `default` is used in a "path" expression, the template engine receives `default()` instead of `default` and (understanably) does not produce the intended result. I see two possibilities to overcome this: * use a non callable "importable symbol" Potentially it is sufficient for an object to have `__module__` and `__name__` attributes to make it into an "importable symbol". At least, this should in principal be sufficient - not sure whether the existing `ast` infrastructure sees it this way, however. * use an appropriate metaclass to ensure that your `DEFAULT_MARKER` class returns itself when called. This can be achieved (in Python 2): ``` class DefaultMeta(type): def __call__(cls): return cls class DEFAULT_MARKER(object): __metaclass__ = DefaultMeta ``` For Python 3, `DEFAULT_MARKER` would be defined by `class DEFAULT_MARKER(metaclass = DefaultMeta)`.

[malthe]

@d-maurer I have pushed 1aa5b40, which changes it from a class to a module.

[d-maurer]

@malthe has solved the original problem differently.