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

Handle non_neg_integer() + pos_integer() :: pos_integer() properly #465

Merged
merged 5 commits into from
Sep 28, 2022

Conversation

erszcz
Copy link
Collaborator

@erszcz erszcz commented Sep 27, 2022

Unfortunately, with the current arithmetic operator support it's not possible to handle

pos_integer() + non_neg_integer() :: pos_integer()

which is a bit unfortunate as addition ought to be commutative 😅

I think that actually unifying infix operator support with erlang:Op functions and using intersection specs/types for the latter might help here, but I'm not sure yet what we could lose going that way.

@josefs
Copy link
Owner

josefs commented Sep 27, 2022

Is this actually an improvement? In situations like these, I tend to just admit defeat and say "Sorry, we can't handle that" rather than make something that is more complicated, harder to understand and only works under some circumstances.
Do you have people asking for this feature?

@erszcz
Copy link
Collaborator Author

erszcz commented Sep 27, 2022

Do you have people asking for this feature?

Not really, but it's already present in Gradualizer and is buggy. The pattern, though, is apparently common enough to be found in places like the Elixir standard library:

  @spec keydelete([tuple], any, non_neg_integer) :: [tuple]
  def keydelete(list, key, position) when is_integer(position) do
    :lists.keydelete(key, position + 1, list)
  end

There's also a few cases in #464, though literally only a few. I'm not arguing that this check is extremely useful, but given it's there it would be nice if it didn't raise false positives. One way to make it happen is to fix it, another way is to remove it.

Copy link
Owner

@josefs josefs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your point about the n + 1 case is pretty compelling. It would indeed be nice to handle that. Given that everyone writes n + 1 and not 1 + n is a good reason to have this feature.
Yeah, you've convinced me that this is a good idea.
Perhaps you can add some example involving n + 1 in the documentation to motivate the code?

@erszcz erszcz merged commit 32f8604 into josefs:master Sep 28, 2022
@erszcz erszcz deleted the non-neg-plus-pos-is-pos branch September 28, 2022 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants