Force arguments that are used in closures

[krlmlr]

The reprex below triggered the investigation that led to possible instances where "brittle closures" are returned. I have no idea if the other instances I found have an effect, but it feels like good practice to force arguments that are used by closures.

Happy to add tests as necessary.

library(tidyverse)

scale <- c("red", "green", "blue")

p <-
  ggplot(iris, aes(Petal.Length, Petal.Width, color = Species)) +
  geom_point() +
  scale_color_manual(values = unlist(scale), labels = levels(iris$Species))

scale <- scale[-1]

p

Created on 2018-08-07 by the reprex package (v0.2.0).

[hadley]

No need for tests, I think.

[clauswilke]

@krlmlr Do you want to look whether there are any other such cases or do we leave it as it currently is for this PR?

[clauswilke]

For my own education, could you explain why we now need filename = NULL here but in some other cases (e.g., make_summary_fun()) we don't need to add a default?

[krlmlr]

The default doesn't make sense there to me, the function needs arguments or fails. But e.g. manual_scale() could use a better error message.

[krlmlr]

This PR contains all instances I've found, looking for ^ +function in the code and ignoring false positives. The initial comment describes just one instance. This is good to go from my end.

[krlmlr]

NEWS entry:

• Functions that generate other functions (closures) now force the arguments that are used from the generated functions, to avoid hard-to-catch errors (Force arguments that are used in closures #2807, @krlmlr).

[hadley]

I don't think this warrants a news bullet as the chance of it affecting user code is extremely small.

[krlmlr]

My example came from user code, though. I guess it would fit in a change log but not in NEWS.

[clauswilke]

Thanks!

[lock]

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/