-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
manual: methods: new section on avoiding ambiguity: "playing nicely" #58005
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
base: master
Are you sure you want to change the base?
manual: methods: new section on avoiding ambiguity: "playing nicely" #58005
Conversation
|
I think the information & reminder of the possibility of ambiguity could definitely be useful to have in the docs. But in my opinion I would phrase it more neutrally-informatively as a reminder rather than as an admonition. that would mean bringing the tone from "don't do this as it's impolite" ---> "if you do this, ambiguities can occur" after all, maybe the package authors are completely fine with creating |
|
The issue is such: an "impolite" package causes method ambiguity with respect to an unbounded number of potential packages. We can't expect either:
So the gotcha you mention, where the two packages resolve the ambiguity among themselves, without considering the rest of the ecosystem, is valid only when these two packages are in a special relationship where one of the two packages (the dependent package) depends (either strongly or weakly) on the other package (the dependency package) and the dependent package is allowed to access private parts of the dependency package. Such a relationship indicates that the ownership between the two packages is the same or shared. And that's why I included the "unrelated ownership" wording, although the clarity may need to be improved still. |
|
Thinking about the gotcha more, I think it would be fine not to mention it in the docs. The gotcha is basically "I defined a method that causes ambiguity, but you don't need to care about it because an ambiguous call may only happen if you access my internals". However that's still impolite because of causing Aqua.jl noise in my opinion. |
|
I'm not sure how to generically discuss this because it depends upon how everyone defines their methods and what the "interface" is. For example, both We definitely can't say that |
The fact that an important (for the ecosystem) registered package failed to follow the advice/rule provided in the change is evidence to the claim that adding these docs is important. Ref: * JuliaAlgebra/MultivariatePolynomials.jl#310 Fixes JuliaLang#55866
8f5a417 to
07006e2
Compare
|
👍 Describing the conditions where this rule applies seems difficult, so I guess I'll adjust this PR to be just a collection of examples, without the general rule, when I get back to it. |
| * in another (polite) package: | ||
| ```julia | ||
| struct Y <: AbstractFloat end | ||
| function Base.:(==)(::Integer, ::Y) end | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand, in how far is this more "polite" than the one above? They both seem to be equally "bad"?
| * in another (polite) package: | ||
| ```julia | ||
| struct Y <: AbstractFloat end | ||
| function Base.:(==)(::Integer, ::Y) end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it is "polite" because Y is not a subtype of Integer? perhaps this could be made explicit via a comment:
| function Base.:(==)(::Integer, ::Y) end | |
| function Base.:(==)(::Integer, ::Y) end # this is fine as Y is not a subtype of Integer |
| Then: | ||
| * Do not add a method to `f` such that the type of either argument is not constrained (type `Any`). For example, this is not valid: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which sense is it not "valid"? It certainly is valid Julia code, just "impolite" as you call it later on.
How about
| Then: | |
| * Do not add a method to `f` such that the type of either argument is not constrained (type `Any`). For example, this is not valid: | |
| Then: | |
| * Do not add a method to `f` such that the type of either argument is not constrained (type `Any`). For example, this violates the rule: |
Or perhaps fully lean into this "polite" / "impolite" business:
| Then: | |
| * Do not add a method to `f` such that the type of either argument is not constrained (type `Any`). For example, this is not valid: | |
| Then in order to write "polite" code: | |
| * Do not add a method to `f` such that the type of either argument is not constrained (type `Any`). For example, this is not polite: |
The fact that an important (for the ecosystem) registered package failed to follow the advice/rule provided in the change is evidence to the claim that adding these docs is important. Ref:
Fixes #55866