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

cmd/compile: unexpected type inference with interface types #60933

Closed
ianlancetaylor opened this issue Jun 22, 2023 · 23 comments
Closed

cmd/compile: unexpected type inference with interface types #60933

ianlancetaylor opened this issue Jun 22, 2023 · 23 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone

Comments

@ianlancetaylor
Copy link
Contributor

Example program https://go.dev/play/p/rLW5osbRq6Z?v=gotip

package main

import (
	"fmt"
	"io"
	"os"
)

func F[T any](v ...T) {
	var zero T
	fmt.Printf("%T\n", &zero)
}

func main() {
	var nilFile *os.File
	F(nilFile, io.Discard)
	F(nilFile, os.Stdout)
}

With current HEAD running this program prints

*io.Writer
**os.File

The second result is reasonable. The first, less so. The compiler has taken a value of type *os.File and a value of type io.Writer, which must be of the same type, and inferred a type argument of io.Writer. This seems potentially quite confusing, as pointed out at #60204 (comment).

Let's make sure this is what we want before we commit it to 1.21.

CC @griesemer @findleyr

@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker compiler/runtime Issues related to the Go compiler and/or runtime. labels Jun 22, 2023
@ianlancetaylor ianlancetaylor added this to the Go1.21 milestone Jun 22, 2023
@griesemer
Copy link
Contributor

Analysis:

In the first case (F(nilFile, io.Discard)), we have an unnamed type (*os.File) and a named type (io.Writer, type of io.Discard). One of them (it doesn't matter which one), is inferred as the initial "guess" for the type parameter T. Now the 2nd of the two types must match with T. That is, *os.File and io.Writer must unify. Because we have a named and an unnamed type which are different, type inference tries to unify the underlying types, so *os.File (unchanged) and interface{ Write(p []byte) (n int, err error) } (underlying type of io.Writer). Now we have the unification of an interface and a non-interface type. This succeeds if the non-interface type (*os.File) implements the interface, which it does. Therefore unification of *os.File and io.Writer succeeds.

Now, because io.Writer is a named type, we record io.Writer as the type argument for T (if it's not yet the inferred type argument). This is important in other (non-interface) situations: a named type with the same underlying type as an unnamed type unifies, so it's safe to infer the named type. If we don't do that, inference becomes function parameter order-dependent.

Not yet sure what the right "fix" here is.

@griesemer griesemer self-assigned this Jun 22, 2023
@griesemer
Copy link
Contributor

Do we want an error in this case, such as:

cannot use io.Discard (variable of type io.Writer) as *os.File value in argument to F: need type assertion

?

@bcmills
Copy link
Contributor

bcmills commented Jun 22, 2023

I think this may be more of an issue with cmp.Or than with type inference proper.

The example in #60204 (comment) seems almost as confusing even when the types are explicit:

	var outputFile *os.File
	{
		output := Or[*os.File](outputFile, os.Stdout)
		fmt.Println(output == outputFile, output == os.Stdout) // false true
	}
	{
		output := Or[io.Writer](outputFile, io.Discard)  // oops
		fmt.Println(output == outputFile, output == io.Discard) // true false
	}

Perhaps the right solution is a vet check for calling Or instantiated at an interface type with a possibly-nil argument of a concrete type.

@griesemer
Copy link
Contributor

We probably should never infer an interface type unless all types that need to unify are the same (possibly named) interface. In all other cases, inferring an interface type amounts to finding a "super type" implemented by other, non-interface types; or finding the most general interface type among a set of interface types (which may lead to inferring an interface type that doesn't exist in the program, or to any). Both these scenarios are problematic.

If we follow this approach, we will get an error for the problematic F(nilFile, io.Discard) case.

@atdiar
Copy link

atdiar commented Jun 22, 2023

That's strange. I have some questions.

Is *os.File an unnamed type because it's a pointer?
I would have thought it was named.

In the original issue, I think that the type inference is somewhat correct in inferring io.Writer (modulo what to do when an interface is inferred since many of them could be so I'd think that inference would fail to determine an exact type [edit] @griesemer is addressing that point already just above) but I'm a bit puzzled by the result of the equality between output and outputFile.

These are not variables of the same type, I must be missing or forgetting something but does equality works on variables of different types?

Maybe if these questions can help.

@DeedleFake
Copy link

DeedleFake commented Jun 22, 2023

I think this may be more of an issue with cmp.Or than with type inference proper.

The cmp.Or() case is an extension of the long-standing confusion around non-nil interfaces wrapping a nil value. The inference from the second argument causes the first to have different behavior. For example,

func IsFirstZero[T any](v1, v2 T) bool {
  var zero T
  return v1 == zero
}

func main() {
  fmt.Println(IsFirstZero((*os.File)(nil), os.Stdout)) // true
  fmt.Println(IsFirstZero((*os.File)(nil), rand.Reader) // false
}

Thanks to the inference to an interface type, the first is no longer a zero value when it was before. This can happen without any explicit typing, making it extremely confusing and hard to track down. At the very least I think it should be an error to infer an interface type from multiple sources where at least one is not an interface type. Then you'd have to do an explicit conversion, such as IsFirstZero(io.Reader((*os.File)(nil)), rand.Reader). I think the only real alternative is to modify the way nil is defined for interfaces, and that seams unreasonable.

@hherman1
Copy link

Does it really solve the problem? It seems like you would actually want to be able to say “pick the first non-nil reader” and even if you make type inference manual, it’s still not correct to pass a nil OS file here

@earthboundkid
Copy link
Contributor

I feel like if you have func F[T any](a, b T) and var c someconcrete; var i someiface, it really should force you to write either F(someiface(c), i) or F[someiface](c, i). Just promoting c to an interface automatically is a little too surprising because you get different inferred types for F(c, c) and F(c, i).

@earthboundkid
Copy link
Contributor

In a quick test, this does the inference if a and b are *os.File and io.Reader, but not if it's some type and any. That also feels like an inconsistency if not an outright bug. https://go.dev/play/p/GBw94NAuJgD?v=gotip

@mdempsky
Copy link
Contributor

I'd lean towards making it an error to implicitly convert from a concrete type to an inferred interface type. That would make cmp.Or(nilFile, io.Discard) into an error (e.g., "cannot assign nilFile (type *os.File) to inferred interface type io.Writer"), while still allowing cmp.Or[io.Writer](nilFile, io.Discard) and cmp.Or(io.Writer(nilFile), io.Discard) (not that I think either of these forms would be useful with cmp.Or).

That seems like the minimal restriction to protect users from the mistake of accidentally converting zero-values of concrete type into non-zero-values of interface type, due to type inference.

@griesemer
Copy link
Contributor

@mdempsky Is your suggestion different from what I wrote here?

@mdempsky
Copy link
Contributor

@griesemer As I understand your previous message, F(io.Writer(nil), io.ReadWriter(nil)) would be an error, because io.Writer and io.ReadWriter are distinct interface types.

I think making that an error is conservative and okay for 1.21.

But I'd argue that I think it's okay to infer io.Writer as the type argument, because io.ReadWriter is still an interface type. So the implicit conversion from assigning the io.ReadWriter argument to the io.Writer parameter type can't turn a zero value into a non-zero value. The error-prone zero-into-non-zero conversion only happens when turning a value of concrete type into an interface type.

--

Incidentally, I notice that F(io.Writer(nil), io.ReadWriter(nil)) and F(io.ReadWriter(nil), io.Writer(nil)) infer different type arguments: https://go.dev/play/p/JhwnFnwxOeA?v=gotip

Is that intentional/known? I thought inference was supposed to be independent of argument order.

Perhaps related to #60946.

@griesemer
Copy link
Contributor

@mdempsky Ok, thanks for the clarification. Inference ought to be independent of argument order. I think the problem you describe is related to #60946 but I will verify.

@griesemer
Copy link
Contributor

@carlmjohnson In your comment, note that any is not a named (= defined) type, it's an alias for interface{}. In that case, it doesn't get selected over a previously inferred unnamed type that unifies with any. That explains the behavior; but I agree that this is wrong and needs to be fixed (it's part of this very problem).

@DmitriyMV
Copy link
Contributor

DmitriyMV commented Jun 22, 2023

I'm not sure if observed behavior is actually a bug. If I do something like this I would expect the result to be io.Writer. But trying further I'm not sure I would expect this code to compile, let alone infer to map[main.myAny]struct {}.

The main problem I see is that if I change some of the arguments to the different types (by adding or removing methods) it will start returning different types, which is not ideal on big codebases.

But still - I would prefer var set map[io.Writer]struct{} = toSet(os.Stdout, os.Stdin, io.Discard, os.Stdout) or this to work without explicit type parameter passing.

@griesemer
Copy link
Contributor

griesemer commented Jun 22, 2023

The observed behavior is clearly a bug. If we use a Discard with an unnamed Writer interface, swapping the arguments leads to different results. This is related to this issue. See also #60946.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/505396 mentions this issue: go/types, types2: fix interface inference

@griesemer
Copy link
Contributor

griesemer commented Jun 23, 2023

Proposed solution:

  • Inference fails whenever we need to select a type from a list of types that includes interface and non-interface types (this addresses the issue at hand). (We can probably make this work correctly, but not for 1.21. By failing in this case we leave the door open to fixing it down the road.)

  • If we have only interface types, the most general interface is selected, if there is one in the list that is implemented by all other interfaces that appear. (We don't want to create interfaces that do not appear in the program.) This addresses @mdempsky 's comment.

  • If we have only interface types, they must have the same methods. We cannot chose the most general one because that may introduce an order dependency since we also select named interfaces over unnamed ones.

  • If all interfaces are the same, and there are named interfaces, the named interfaces must all have the same name or inference fails. (If we have to chose a name, inference becomes parameter-order dependent. This would be visible when the type is printed.)

  • If a named interface exists (and all other named interfaces have the same name), the named interface is selected over unnamed interfaces with the same methods (but if the interfaces have different method sets, the most general one wins, see first rule above).

The actual change is very localized and small.

This should address this issue and also #60946, and is backward-compatible with 1.20.

@DmitriyMV
Copy link
Contributor

@griesemer

Inference fails whenever we need to select a type from a list of types that includes interface and non-interface types (this addresses the issue at hand)

Will this https://go.dev/play/p/FsmiNHCX-IA?v=gotip (toSet inferring from the left side of the expression) work after this change?

@griesemer
Copy link
Contributor

@DmitriyMV No, because io.Discard is an interface type. That is exactly the problem we need to address here. You will need to instantiate explicitly, as in

var set mySet = toSet[io.Writer](os.Stdout, os.Stdin, io.Discard, os.Stdout)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/505395 mentions this issue: go/types, types2: more readable inference trace

gopherbot pushed a commit that referenced this issue Jun 26, 2023
Print the unification mode in human-readable form.
Use a tab and // instead of ()'s to show unification mode
and whether operands where swapped.

These changes only affect inference trace output, which is
disabled by default. For easier debugging.

For #60933.

Change-Id: I95299c6e09b90670fc45addc4f9196b6cdd3b59f
Reviewed-on: https://go-review.googlesource.com/c/go/+/505395
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/506396 mentions this issue: go/types, types2: simpler fix for interface unification

bradfitz pushed a commit to tailscale/go that referenced this issue Jul 15, 2023
Print the unification mode in human-readable form.
Use a tab and // instead of ()'s to show unification mode
and whether operands where swapped.

These changes only affect inference trace output, which is
disabled by default. For easier debugging.

For golang#60933.

Change-Id: I95299c6e09b90670fc45addc4f9196b6cdd3b59f
Reviewed-on: https://go-review.googlesource.com/c/go/+/505395
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
bradfitz pushed a commit to tailscale/go that referenced this issue Jul 15, 2023
When unification of two types succeeds and at least one of them
is an interface, we must be more cautious about when to accept
the unification, to avoid order dependencies and unexpected
inference results.

The changes are localized and only affect matching against
interfaces; they further restrict what are valid unifications
(rather than allowing more code to pass). We may be able to
remove some of the restriotions in a future release.

See comments in code for a detailed description of the changes.

Also, factored out "asInterface" functionality into a function
to avoid needless repetition in the code.

Fixes golang#60933.
Fixes golang#60946.

Change-Id: I923f7a7c1a22e0f4fd29e441e016e7154429fc5e
Reviewed-on: https://go-review.googlesource.com/c/go/+/505396
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@jub0bs
Copy link

jub0bs commented Aug 8, 2023

@atdiar

Is *os.File an unnamed type because it's a pointer?
I would have thought it was named.

According to the spec, os.File is a named type, but *os.File is not a named type:

Predeclared types, defined types, and type parameters are called named types. An alias denotes a named type if the type given in the alias declaration is a named type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Development

No branches or pull requests