Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove ambiguity in nullary remote calls with unknown targets #9510

Open
2 of 3 tasks
josevalim opened this issue Nov 8, 2019 · 11 comments
Open
2 of 3 tasks

Remove ambiguity in nullary remote calls with unknown targets #9510

josevalim opened this issue Nov 8, 2019 · 11 comments

Comments

@josevalim
Copy link
Member

josevalim commented Nov 8, 2019

  • Deprecate map.foo() in favor of map.foo
  • Deprecate mod.foo in favor of mod.foo()
  • Change the AST so remote calls (A.b, a.b, etc) without parens has nil third element. Once we do so, remove :no_parens metadata usage throughout the codebase (perhaps behind the --future flag)

PS: originally this issue was about deprecating all nullary remote calls without parens, but it has been restricted since then. See this comment for more info.

@eksperimental
Copy link
Contributor

eksperimental commented Nov 10, 2019

What about typespecs of arity 0?

@josevalim
Copy link
Member Author

@eksperimental we probably won't touch typespecs for two reasons:

  1. of all of the examples above, it is the most common, which means potentially hundreds of warnings in a project

  2. it can't be fully automated, which doesn't help with 1

@esse
Copy link
Contributor

esse commented Nov 20, 2019

Is this open for contribution? Because I would love to work on that :)

@josevalim
Copy link
Member Author

Hi @esse, not yet. Note it is tagged for v1.11 and we are still working on v1.10.

@josevalim
Copy link
Member Author

josevalim commented Mar 12, 2020

Folks, I will go ahead and revert the changes to the pipe operator. I have been running them for the last 40 days and I have to be honest that every time I saw the warning a part of me died a bit. :)

There is nothing ambiguous on |> Foo.bar, so it makes no sense to push users to use parens. That's opposite to an unqualified bar without parens, which was ambiguous and it required revisiting the context.

Therefore, we will only apply these rules where ambiguity exists: map.foo() and mod.foo. We will likely start emitting a warning for map.foo() first and a warnings for mod.foo later.

EDIT: I have updated the original description to track only the changes we will effectively do.

@josevalim josevalim changed the title Deprecate nullary remote calls Remove ambiguity in nullary remote calls with unknown targets Mar 12, 2020
@josevalim josevalim added this to the v1.11.0 milestone May 15, 2020
@josevalim
Copy link
Member Author

On v1.11 we will add this as a compile-time warning whenever we can infer it. So I will postpone this to v1.12 or maybe even v1.13.

@alco
Copy link
Member

alco commented Aug 31, 2023

Hey. Just wanted to loop back on this issues since it's been up since 2019 and was last slated to be addressed in v1.13. We're at v1.15.5 now and no warning is emitted for code like this:

fun = fn -> IO.puts "Hey there" end
[%{ack_fn: fun}] |> Stream.each(& &1.ack_fn()) |> ... |> Enum.to_list()

That's a simplified version of what we had in our product's codebase until recently. Because of the missing . in &1.ack_fn(), the ack function wasn't getting called as it was supposed to.

@josevalim
Copy link
Member Author

PRs are welcome! :)

@viniciusmuller
Copy link
Contributor

I would like to take a look at this, but I think I'm not familiar enough with the context? Would it be possible to add a bit more of context on some code samples we should warn and others that where we shouldn't?

For example, for the case above it's not clear to me what would be the criteria for warning, as the type should be unknown and &1 could be a module, making it a valid call

@alco
Copy link
Member

alco commented Sep 17, 2023

I'm thinking it could be a warning saying something to the tune of "Unable to determine the type of &1. If it is a module, use apply(&1, :ack_fn, []). If it is a map, use &1.ack_fn.()"

@josevalim
Copy link
Member Author

Sorry, I dropped the ball on these comments, but the deprecation is now on main.

@josevalim josevalim added this to the v2.0 milestone May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants