Shopify
`blank` and `empty` special keywords are not properly parsed in Liquid 5
Hey friends,
I was just working on upgrading Jekyll from Liquid v4 to v5. I have everything working except our where filter. We have our own where filter for arcane historical reasons. It works flawlessly in Liquid 5 except in two cases: using empty or blank as the value comparator:
{{ hash | where: "property", empty }}
{{ hash | where: "property", blank }}
We expect these to somewhat behave like Ruby's empty? and Rails's blank? methods, since previously Liquid would pass us the Liquid::Expression::MethodLiteral corresponding to these special keywords. We could then process them specially. Unfortunately, we're now just getting the empty string ("") when we use blank or empty. (https://github.com/jekyll/jekyll/pull/9030#issuecomment-1094510678)
I see that @dylanahsmith (👋) wrote #1300 which changed Liquid::Expression::LITERALS to set empty and blank as "" which could be related to why our filter is getting target = "" instead of target = Liquid::Expression::MethodLiteral<name=:empty? to_s="">.
Is there any way we, as the filter authors, could be notified that the target is the special keyword empty or blank?
Thanks for your hard work on Liquid!
Oh, I wasn't aware that filters had been receiving this special Liquid::Expression::MethodLiteral object, given the to_liquid implementation on that class returning an empty string. As in, it wasn't actually a liquid value, it just supported being cast to the "" liquid value. For instance, if the value were assigned to a variable and then used, it would silently change to an empty string.
These special keywords were only meant to be used internally in conditional expressions, but by them being returned from Expression.parse and those expressions being passed directly into filters as values, filters ended up being relied upon to cast these values to strings. This created the illusion that the abstraction wasn't leaking, but this leaky concern added a fragile edge case, which could cause problems if any filter used an .is_a?(String) check on one of these values without considering these normally string values.
Not only was this a leaky abstraction, but liquid has also been pretty bad at documenting what is and what isn't meant to be a part of the public API. Given this was only implicitly internal, it is kind of understandable that it ended up being depended upon.
A hacky way of working around the problem could involve monkey patches, relying on the fact that having Liquid::Expression.parse return a MethodLiteral would work as it previously did if a to_liquid method is added back to it. However, this is just another form of internal coupling which would be fragile to liquid implementation changes, so doesn't seem like a maintainable solution. It would also interfere with liquid-c compatibility.
The primary purpose for #1300 was to simplify the introduction of expressions to the liquid-c VM. If liquid-c added support for those method literals, then it would be possible to revert that change and keep liquid-c compatibility. However, it still would be a shame to regress back to having this edge case to worry about in the majority of cases where it isn't desirable, so ideally this behaviour could be limited to where it is desirable.
Longer term, I would like to be able to annotate liquid extensions (including filters) with type information for static analysis, which could offer a way of specify that a filter accepts a method literal in a given parameter.
@dylanahsmith Thanks for the background. I agree that this seems like it was a mistake originally, but it offered Jekyll (and presumably other filter creators) semantic meaning. For example, we could differentiate between value === "" and value.empty? and value.blank?. The first allowed us to explicitly match an empty string when the user requested it. The second and third allowed us to match nil, {}, and [], too. By collapsing this MethodLiteral into "", we must give up either the strict equals with "" (since we'd need to match empty collections too), or we'd need to give up matching empty collections. When I'm writing a Liquid template, I would expect empty and blank to work on collections, but I would not expect "" to work on collections. I don't know if we have a good way forward since we do want to allow liquid-c compatibility here. Preserving some kind of differentiation between "a literal empty string" and a generic "is this empty/blank" seems like it most closely matches the user's expectations. Why would we have empty and blank otherwise when a user could just as easily use ""?
@dylanahsmith Is there any way that Liquid 5 could indicate whether it's asking for permissive equality (e.g. [] == "") versus strict equals (e.g. [] !== "")? A straight-up empty string breaks some users without this information, which the user has semantically provided.
Yes, I understand how this makes sense in Jekyll.
I don't know if we have a good way forward since we do want to allow liquid-c compatibility here.
I'm sorry I wasn't clear enough when I said the following
If liquid-c added support for those method literals, then it would be possible to revert that change and keep liquid-c compatibility.
I mean we should expose the MethodLiteral to filters again. liquid-c should be updated to be consistent with this repo.
We should probably consider whether it should be treated as a liquid value for consistency. As in, MethodLiteral#to_liquid would return self, which would allow the semantic value to be preserved when assigned to another variable.
I do still have concerns that this could cause problems with some code that is doing an is_a?(String) check, such as to handle a polymorphic argument in a different way depending on its type. However, any code dealing with a parsed expression directly (e.g. filters) would have to have already handled this case if it was written for liquid 4. If any bugs come up from this, then I think we should be able to fix forward. In most cases, value coercion should be sufficient, where we could even have other coercion methods like to_a that match the semantics of empty.
If we stop there, the empty and blank values would still seem off for the builtin where filter if they were used as a target value. Instead, I think we should consider extending the liquid where filter to support those values according to their semantics as well, since it makes sense that it would use comparison semantics, just like the conditional tags. This would also make this feature easier to support, by having it be something that can be tested using a standard liquid feature, rather than being one that seems unnecessary without having the context of Jekyll depending on it.
@dylanahsmith youve already done your good deed for the year in releasing liquid 4.0.4 (thank you again!) but let me know if we can proceed on this front! It sounds like there are changes to these literals and to the Liquid where filter needed to patch this.
Can Shopify get behind the changes to the where filter or will that break customers' sites?
I think there is a low enough risk of the change breaking existing liquid code that it would be worth deploying (without a release), similar to how we make bug fixes sometimes that carry a low risk of breakage to make things work as expected. If the change is too disruptive, then we can rollback and decide how to respond accordingly. If it doesn't need to be rolled back or reverted, then we could make a release candidate to invite others to do the same.
I don't know if we have a good way forward since we do want to allow liquid-c compatibility here.
I can help with making the corresponding change to liquid-c