Warn on parentheses in query field access#4746
Conversation
A field access written with empty parentheses such as q.title() is silently accepted and treated identically to q.title. The two only differ by the no_parens: true metadata on the outer call, so bind that metadata and emit a deprecation warning when it is absent, then proceed as before. Closes elixir-ecto#4588. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
|
Thanks for the PR @arpitjain099. I think we have a bunch of tests that unfortunately trigger this warning. If you run locally you will see a lot of these Warning: test/ecto/query/builder/distinct_test.exs:19: Ecto.Query.Builder.DistinctTest."test escape handles expressions and params"/1
warning: using parentheses after a field access, such as `x.x()`, is deprecated and will raise in future Ecto versions. Remove the parentheses to keep the current behaviour: `x.x`and the test is like this assert {Macro.escape(quote(do: [asc: &0.x(), asc: &1.y()])), {[], %{}}} ==
escape(quote(do: [x.x(), y.y()]), {[], %{}}, [x: 0, y: 1], __ENV__)Would you have time to change the tests as well? If not it's ok I can do a cleanup PR right after. |
The new deprecation warning for parenthesized query field access fired across the builder test suite, since many escape/1 cases wrote fields as x.x() instead of x.x on the input side. Drop the empty parentheses so the tests exercise the same expressions without emitting the warning. The escaped output is identical either way, so the assertions are unchanged. The intentional case in builder_test.exs that verifies the warning keeps its parentheses. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
|
Thanks @greg-rychlewski, happy to. I went through the builder tests and dropped the empty parens from the field accesses that were emitting the warning (distinct, filter, group_by, order_by, select, update, windows, and the main builder_test). Those were all on the Ran the full suite locally and it's green again with no more |
|
💚 💙 💜 💛 ❤️ |
3 participants
Closes #4588. Writing a field access with empty parentheses like
q.title()is currently accepted and behaves exactly likeq.title, which is almost always a typo. Per josevalim's note on the issue ("we should definitely warn and then convert to an error in the future"), this warns instead of raising for now.The two forms only differ by
no_parens: trueon the outer call metadata (confirmed in the thread), so the escape clause now binds that metadata and emits a deprecation warning viaIO.warn/2when it is absent, then proceeds unchanged. Added a builder test asserting the warning fires forx.y()and stays quiet forx.y.One heads-up: I don't have an Elixir toolchain on this machine, so I verified by code-read rather than running
mix testlocally, and I am counting on CI here. Also, a handful of existing builder tests use the parenthesized form (x.y()), so they will now print this warning during the run without failing; happy to follow up and quiet those if you would like.