search

plumatic / schema

Clojure(Script) library for declarative data description and validation
Other
2.4k stars 256 forks source link

The CLJS fn-name utility fails if the function name ends in $ #416

Closed zmthy closed 5 years ago

zmthy commented 5 years ago

Our entire client test suite crashes when upgrading its schema dependency to 1.1.11. The cause of the crash is this change:

https://github.com/plumatic/schema/commit/2e72125c4b2d8dac3bbed5bd0ac5592e0dce3ec5#diff-023920e7404f0106d749abcaf369621eR66

An advanced optimization by the Closure compiler was adding $ to either side of the munged name of a function being passed to pred, causing the resulting demunge to end in a slash. The regex [^/]+$ that follows fails to match the string, because there is not at least one non-slash character before the end, so fn-name returns nil. This causes pred to throw because nil cannot be turned into a symbol.

gfredericks commented 5 years ago

Thanks for the detailed report!

Is this easy to write a unit test for?

zmthy commented 5 years ago

Yes, since $ is a valid character in a function name.

Looks like I wasn't quite accurate: it fails if the function ends in at least two $ characters, since demunge will strip a trailing $ but not all trailing $.

Something like this will work:

#+cljs (is (not= nil (utils/fn-name (fn foo$$ [x] (+ x x)))))

It would be better to test what the actual expected output is, but I'm not sure what that should be: foo or function?

gfredericks commented 5 years ago

would foo$$ be bad? Perhaps you're thinking correctly returning foo$$ is impossible because of ambiguity?

zmthy commented 5 years ago

I don't think it's ambiguous because there's no way there could have been slashes at the end of the original name, but it depends how consistent you want to be with a function name like foo$bar, which is ambiguous. Stripping the trailing $s would be ideal for my use-case, because it would undo the rewriting that Closure did, but that doesn't seem like the most general solution. Adding any trailing $s on the original name back to the end of the demunged name before it goes into the final regex is probably the right solution (and that would handle the case of the name only consisting of $s as well).

gfredericks commented 5 years ago

Take a look at #420 and let me know if it addresses your concerns. If not, some additional test cases would be helpful.

zmthy commented 5 years ago

Works for me!

gfredericks commented 5 years ago

Released as 1.1.12.