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

proposal: Go 2: integer comparison casting #35010

Closed
Hucaru opened this issue Oct 19, 2019 · 19 comments

Comments

@Hucaru
Copy link

@Hucaru Hucaru commented Oct 19, 2019

When comparing two integers of differing type the following if statement is required:

var i int32 = 1
var j int16 = 2
if int32(j) > i {
}

My proposal is to implicitly cast the 'smaller' integer (j) to the 'larger' integer (i) type. This would not be allowed between signed and unsigned types. This allows the above to be re-written as:

var i int32 = 1
var j int16 = 2
if j > i {
}

I feel the above has the following advantages over the former:

  1. If integer types change then the statement still holds, whereas the former might not
  2. Helps removes human error, for example the following if statement evaluates to true:
var i int32 = 2147483647
var j int16 = 2
if j > int16(i) {
}
  1. Compatible with current Go and if a user wants to be explicit they still can.
@gopherbot gopherbot added this to the Proposal milestone Oct 19, 2019
@gopherbot gopherbot added the Proposal label Oct 19, 2019
@Hucaru Hucaru changed the title proposal: integer comparison proposal: integer comparison casting Oct 19, 2019
@ianlancetaylor ianlancetaylor changed the title proposal: integer comparison casting proposal: Go 2: integer comparison casting Oct 19, 2019
@ianlancetaylor ianlancetaylor added the Go2 label Oct 19, 2019
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 19, 2019

This is a FAQ: https://golang.org/doc/faq#conversions .

Experience teaches us that explicit is better than implicit.

@Hucaru

This comment has been minimized.

Copy link
Author

@Hucaru Hucaru commented Oct 19, 2019

I think the points it makes are valid and I wouldn't like to see implicit casting for assignments but for comparison I think it's better as long as you keep it between the same numerical representations - can only implicitly cast when comparing a uint with a uint; an int with an int.

To allow bugs (like in the 3rd example) to arise when we could mitigate/prevent them because of a rule stating never allow any numerical casting to be implicit seems rather unfortunate to me.

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Oct 22, 2019

One argument in favor of this is that it would likely result in more correct code in the wild.

With the current situation today, I often see people doing a comparison between an int and an int64 and incorrectly truncating the int64 down to an int, rather than the int up to an int64. It works out fine in practice (on 64-bit arches) but is a bug sitting and waiting to bite 32-bit users.

If we made this language change we'd do the right thing automatically, and the code would look prettier. (fewer conversions).

Edit: durr, this was exactly what you said about "human error" above in bullet (2).

@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Oct 22, 2019

cc: @robpike

@magical

This comment has been minimized.

Copy link
Contributor

@magical magical commented Oct 23, 2019

Rather than define it in terms of casting to a larger type, I would simply say that integer comparisons are done according to the mathematical value of the two integers, as if they were both infinite-precision signed values. Then you could drop the restriction about not comparing unsigned and signed types.

That said, this proposal could lead to subtle bugs involving overflowing counters.
For example, this loop

var limit int64
for i := 0; i < limit; i++ {
  // do stuff
}

might never terminate if it was run on a system where int is 32 bits and limit > (1<<32)

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 23, 2019

this proposal could lead to subtle bugs involving overflowing counters

Note that those bugs are already possible (albeit with fewer non-terminating boundary conditions) today:
https://play.golang.org/p/2a2qTx5xu_a

The only way I can see to reliably avoid subtle overflow bugs is to disallow implicit overflow (#30209 or related proposals).

@robpike

This comment has been minimized.

Copy link
Contributor

@robpike robpike commented Oct 23, 2019

Brad makes a good point. It's an interesting idea. The spec would get trickier, but maybe not too much. On the fence but interested.

quasilyte added a commit to go-critic/go-critic that referenced this issue Oct 25, 2019
Checks when numeric types are compared with truncated conversion on
the either side.

Problematic code example:
	func f(x int32, y int16) bool {
	  return int16(x) < y
	}

Fixes code example:
	func f(x int32, int16) bool {
	  return x < int32(y)
	}

See golang/go#35010

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>
quasilyte added a commit to go-critic/go-critic that referenced this issue Oct 25, 2019
Checks when numeric types are compared with truncated conversion on
the either side.

Problematic code example:
```
	func f(x int32, y int16) bool {
	  return int16(x) < y
	}
```

Fixed code example:
```
	func f(x int32, int16) bool {
	  return x < int32(y)
	}
```

See golang/go#35010

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>
@quasilyte

This comment has been minimized.

Copy link
Contributor

@quasilyte quasilyte commented Oct 25, 2019

@Hucaru I feel like this task can be solved with static analysis. If someone does this kind of a cast inside comparison, linters can warn about it.

Introducing implicit casting into a language where external static analyzers will be enough seems sub-optimal to me.

(The case pointer out by @bcmills is much harder to determine in general, but whole-program analyzers like PVS for C++ do that with quite good results.)

quasilyte added a commit to go-critic/go-critic that referenced this issue Oct 25, 2019
Checks when numeric types are compared with truncated conversion on
the either side.

Problematic code example:
```
	func f(x int32, y int16) bool {
	  return int16(x) < y
	}
```

Fixed code example:
```
	func f(x int32, int16) bool {
	  return x < int32(y)
	}
```

See golang/go#35010

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>
@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Oct 25, 2019

Paging @dominikh for static checkers. (Maybe one already exists?)

@cristaloleg

This comment has been minimized.

Copy link

@cristaloleg cristaloleg commented Oct 25, 2019

@bradfitz looks like it already exists go-critic/go-critic#878 😉

@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Oct 25, 2019

Today, the rules for comparison operators are slightly different from other operators, but for numeric types the rules end up effectively being the same: For comparisons the spec says that one of the operands must be assignable to the type of the other. For integer types this means that the operands have to be effectively the same (excluding untyped operands and shifts for which we have special rules). This is the rule we have for other binary operators: both operands must have the same type.

Since comparisons have special rules already, we can perhaps (but see below) tweak those rules w/o affecting anything else.

That said, I think it's a slippery slope: Once we allow say i16 < i32 (with i16, i32 operands of type int16 and int32), it's quite natural to ask why i16 + i32 should not be permitted. And by extension, why i32 = i16 should not be permitted as well. In fact, if we allow such assignments, the rules for comparison don't have to be changed at all, since changing the assignment rule would change the comparison rule (which is based on assignability) and it would just fall out naturally.

So I think another way of asking this question is: Should (by way of example) i32 = i16 be permitted (and we let the rest follow)?

Perhaps that's a step too far and only comparison rules should be relaxed as proposed. But even that is not so simple. For instance, the code:

var x interface{} = byte(0)
y := 0
result := x == y

is valid and result is false because the dynamic type of x (byte) and the type of y (int) don't match. We couldn't change this behavior without loosing backward-compatibility, but arguably this comparison would then work differently if we allowed byte(0) == int(0) to be ok.

One final observation: Let's assume i32 = i16 were permitted, this would have some consequences on our current contracts (generic) proposal, specifically type inference for parameterized functions: Currently, the proposed inference mechanism is extremely straight-forward. If "widening assignments" are permitted, one would also expect them to be used in type inference which will make the type inference algorithm less straight-forward (an inferred parameter type T is going to be the "smallest (numeric) type large enough" to cover all arguments assigned to parameters of type T).

Just to be clear, I am not for or against this proposal at this point. Just trying to explore the lay of the land.

@dominikh

This comment has been minimized.

Copy link
Member

@dominikh dominikh commented Oct 25, 2019

@bradfitz looks like it already exists go-critic/go-critic#878 wink

You wake up late once and go-critic steals your thunder :-)

@Hucaru

This comment has been minimized.

Copy link
Author

@Hucaru Hucaru commented Oct 25, 2019

@griesemer Excellent points. Your example demonstrates perfectly why this is a potentially precarious language change. @quasilyte raises a very good point that the majority of the benefits that would arise from implementing the proposal could be achieved with static analysis. However, I would like to prevent the situation where bug prevention/mitigation is offloaded to external tools (that could start to make them feel mandatory) when it could be implemented at a language level - I appreciate that this viewpoint could very well be at odds with the direction the Go team wishes to go down.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 25, 2019

So I think another way of asking this question is: Should (by way of example) i32 = i16 be permitted (and we let the rest follow)?

My 2¢: clearly it should be. The destination can exactly represent all of the values of the source without rounding. The requirement for an explicit conversion in such an assignment adds no new information, and prevents no errors.

it's quite natural to ask why i16 + i32 should not be permitted

That one is also (IMO) clear: it should not be permitted unless we also remove silent overflow, because the result of i16 + i32 may today be a different number from i16 + i16 with the exact same numerical values as operands.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 26, 2019

We can't permit i32 = i16 unless we change the rules for defined types or we decide that int32 and int16 are not defined types.

@alanfo

This comment has been minimized.

Copy link

@alanfo alanfo commented Oct 26, 2019

To my mind, the arguments in the FAQ about not allowing implicit numeric conversions remain clear and convincing.

Although this proposal (and the extended idea of allowing implicit widening conversions for assignments as well) would undoubtedly be convenient and might prevent the odd bug, it just doesn't seem worthwhile to me to jeopardize the simplicity and consistency of the present position.

Also, if it were allowed for integers, then no doubt people would ask why it wasn't possible for floats as well when one has a float64 and float32 either side of a comparison (or assignment) operator. So the slope may become even slippier.

@DmitriyMV

This comment has been minimized.

Copy link

@DmitriyMV DmitriyMV commented Oct 27, 2019

Given the points raised by @quasilyte and @griesemer I think it would be wise to have this in go vet. Since there is a lot of working code that actually do int(int64()) maybe it would be a good idea to hide this behind flag like, like there is a flag to check the variable shadowing?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 5, 2019

There seem to be too many complexities to make this work generally.

It seems potentially confusing to permit mixing-and-matching different defined types. Right now Go does not permit comparing a value of type Temperature to a value of type Money. We don't want to start permitting that just because Temperature is defined as int16 where Money is defined as int64. Since int16 and int64 are themselves defined types, there is no obvious way to permit int16 < int64 but not Temperature < Money. Unless we start treating the builtin types as having special properties, which we currently do not (except that they are predeclared).

There is also the interface issue mentioned above. We currently permit comparing interface values using == and !=, and that tests whether the types of the values stored in the interfaces are the same. If we make this change then logically we should extend that comparison to work for comparing values of integer types stored in interface variables, but that also seems complex.

The core idea here can likely be captured in a vet check and apparently go-critic already has that check.

For these reasons this is a likely decline. Leaving open for four weeks for final comments.

-- for @golang/proposal-review

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 3, 2019

No final comments. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.