Feedback seems to accept _ in the N position

[countvajhula]

User 1e1001 on Discord pointed out that an expression like this is accepted:

(~> (1) (feedback _ add1))

This ends up expanding to:

(#%app
 (#%app
  compose
  (flow (feedback _ (then _) add1)))
 1)

and then to: [Edit: added this intermediate expansion to illustrate the problem more clearly]

(#%app
 (#%app
  compose
  (flow (~> (esc (power _ (flow add1))) _)))
 1)

which then is treated as a fancy-app template, eventually becoming:

((compose
  (compose
   values
   (lambda (_1) (power _1 add1))))
 1)

This does not do what is expected (it returns the function add1 as the output).

Possible resolution: It should probably look for _ being passed in the N position and raise a compile-time error in this case.

[rocketnia]

Many Racket syntaxes look for literals with free-identifier=?, including fancy-app (which uses (~literal _) to do so). Using syntaxes like these in the expansion of a macro is hazardous because they can end up looking for literals in the input that the macro author forgot they would look for, allowing code injection bugs like the one you're seeing.

What I'd like to recommend is to avoid using these syntaxes in macroexpansions in places where they'd receive the macro's input as their own input; instead use syntaxes that have a more standard (or more visibly nonstandard) treatment of their subexpressions. In this case I might import the regular #%app under a different name and use that here instead of the fancy-app one. Perhaps #%plain-app would also work here, if there are no keyword arguments.

It seems somewhat frustrating that with fancy-app, it's the function call syntax #%app itself that's hazardous to use in macro templates. Since it's so implicit, code injection bugs caused by #%app are likely to be even harder to spot and fix than those caused by other syntaxes. I feel like this is fixable in the design of fancy-app by doing some scope comparison (bound-identifier=?) to ensure the _ literal was introduced by the same macroexpansion that introduced the function call, but this would technically be a breaking change and would be inconsistent with all the other things in the Racket ecosystem that use free-identifier=? to recognize their literals.

[countvajhula]

Interesting observation! And that's a good call that this is a general class of bug that could occur, and one to watch out for.

Re: avoiding providing macro input to third party syntax that looks for literals (i.e. fancy-app): if none of the expansion rules match, Qi currently does essentially delegate to fancy-app, since anything that expands to a function application gets applied by fancy app's #%app (as that is the #%app in the library module scope).

Identifying each case of invalid Qi syntax that may fall prey to code injection by fancy-app, and raising a compile-time error in these cases, could be one way to handle this (some existing error messages are done this way, example), but it doesn't seem like a great way here. I like your suggestion much better!

Would the following work:

1. Instead of importing #%app from fancy-app via (require fancy-app), do it as something like (require (prefix-in f: fancy-app))
2. In the application template rule specifically, i.e. this rule, expand (func v ...) to a use of (f:#%app func v ...)
3. In all other cases, expansion to a function application would get handled by the default #%app in racket/base by virtue of the module header.

I've never directly used a #% macro (as in (2)) in code - I assume that should be fine.

WDYT?

[rocketnia]

WDYT?

Cross-posting my response to #37 just now:

Ooh, excellent! Yes, I think this approach is a great one if you don't need to make extensive use of fancy-app within the Qi internals. I'm glad it turned out this quick to fix.

[countvajhula]

Fixed in #37