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: spec: disallow impossible interface-interface type assertions #38907

Open
griesemer opened this issue May 6, 2020 · 6 comments
Open
Assignees
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) LanguageChange Suggested changes to the Go language LanguageChangeReview Discussed by language change review committee Proposal
Milestone

Comments

@griesemer
Copy link
Contributor

Now that we have introduced a vet check for this (see #4483), once we use modules to select the language version, we should disallow impossible interface-interface type assertions.

A quick refresher: Given a variable x of type interface I1 with a method m(), and an interface I2 with a method of the same name but different signature m(int), the type assertion x.(I2) can never succeed because no value satisfying I1 can also satisfy I2. The compiler can statically detect that this type assertion makes no sense (it will always panic or return false).

@griesemer griesemer self-assigned this May 6, 2020
@griesemer griesemer added the LanguageChange Suggested changes to the Go language label May 6, 2020
@griesemer griesemer changed the title spec: disallow impossible interface-interface type assertions proposal: spec: disallow impossible interface-interface type assertions May 6, 2020
@gopherbot gopherbot added this to the Proposal milestone May 6, 2020
@griesemer griesemer modified the milestones: Proposal, Go2 May 6, 2020
@ianlancetaylor ianlancetaylor added the v2 An incompatible library change label Jan 6, 2021
@griesemer
Copy link
Contributor Author

griesemer commented Feb 8, 2022

With generalized interfaces (which are not just constrained by methods, but also by types), we have a chance to address this without giving up backward compatibility. For instance, the following:

func _[P interface {
	~int
	m()
}](x interface{ m(int) }) {
	_ = x.(P)  // should this be permitted?
}

is currently permitted, but arguably it shouldn't: it's impossible that the dynamic type of x implements P: whatever the dynamic type of x, it must have a method m(). That type can't implement P which requires m(int).

For a type assertion x.(T) where the type of x is the interface V, I am suggesting the following rules:

  1. If T is not an interface, the type assertion is valid if T implements V. This is another way of saying that T must be in the type set of V, or that the intersection on the type sets of T and V are not empty. This behavior is unchanged from existing Go rules.
  2. If T is a plain non-type parameter interface (i.e., the interface is only constrained by methods), x.(T) is always valid, per existing Go rules. We can't change this because we must preserve backward compatibility.
  3. If T is a generalized or type parameter interface, x.(T) is valid if the intersection of the type sets of T and V are not empty. (If the intersection is empty, it's impossible for any dynamic type of x to implement T).

This can be simplified to:

A type assertion x.(T) is valid if the intersection of the type sets of the type of x and T is not empty. As an exception, for backward-compatibility, the assertion is also valid if T is a plain, non-type parameter interface.

cc: @ianlancetaylor @findleyr

@findleyr
Copy link
Contributor

findleyr commented Feb 8, 2022

@griesemer I am not sure. We are generally permissive of empty type sets, IIRC to avoid action at a distance.

What would we do in the following case?

type C interface {
	~int | string
}

type D interface {
	~float64
}

func _[P interface {
	C
	D
}](x interface{}) {
	_ = x.(P)
}

It seems that by your definition the assertion x.(P) must be disallowed, even though we otherwise permit P to have an empty type set.

@griesemer
Copy link
Contributor Author

griesemer commented Feb 8, 2022

@findleyr So you're saying we should allow this because there's no point in raising an error here since this generic function cannot be instantiated ever (no type implements P). I'd buy this for type parameters.

But if we (in a future version of Go) end up permitting generalized interfaces as variable types, we may want the restriction. For instance, even in non-generic Go we can have

type S struct{}
func (S) m(int)

func _(x interface{ m() }) {
	_ = x.(S)  // invalid type assertion
}

with an invalid type assertion; this is also "action at a distance". If we change the signature of m in S the assertion may go from valid to invalid.

So, to rephrase, the (future) rule might be:

A type assertion x.(T) is valid if the intersection of the type sets of the type of x and T is not empty, or if T is a plain interface (representable as a method set) or a type parameter.

@findleyr
Copy link
Contributor

findleyr commented Feb 8, 2022

@findleyr So you're saying we should allow this because there's no point in raising an error here since this generic function cannot be instantiated ever (no type implements P).

Yes, it seems confusing that no error is otherwise raised for the generic function, except in this innocuous looking type assertion of an empty interface.

A type assertion x.(T) is valid if the intersection of the type sets of the type of x and T is not empty, or if T is a plain interface (representable as a method set) or a type parameter.

Considering this hypothetical future rule, it looks like the spec is strictly more complicated than it would be if we extended our exemption to all interfaces:

"A type assertion x.(T) is valid if the intersection of the type sets of the type of x and T is not empty, or if T is an interface."

Of course, the rule would be even simpler if we didn't have the exemption for plain interfaces in the first place, but we can't change that. This extra complexity in the spec has a real cost in terms of user understanding and tooling support. What benefit do we get to offset this complexity?

with an invalid type assertion; this is also "action at a distance". If we change the signature of m in S the assertion may go from valid to invalid.

I would argue that type set calculations are much less transparent than method signatures. We can produce a very meaningful error message as to why the type assertion fails in that case (missing method or incorrect signature). It is easier for a user to consider the type set when doing an instantiation, versus when doing a type assertion on an empty interface value.

In short, I don't think we should produce an error for type assertion expressions with an empty type set, if we don't produce an error for values with an empty type set.

@ianlancetaylor
Copy link
Contributor

For the record, my personal preference is that these kinds of impossible cases should be rejected at run time and by static analyzers, not by the language itself or the compilers. This is because Go does permit conditional compilation via the use of build tags. It is reasonable for code to be impossible in some configurations and possible in others. Rejecting this code at build times complicates the use of build tags. The gain from blocking these cases in the compiler as opposed to reporting them in go vet is minimal. All just my opinion, of course.

@timothy-king timothy-king added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Feb 9, 2022
@ianlancetaylor
Copy link
Contributor

Putting on hold until we are ready to consider removing language features.

@ianlancetaylor ianlancetaylor added LanguageChangeReview Discussed by language change review committee and removed v2 An incompatible library change Proposal-Hold labels Aug 6, 2024
@ianlancetaylor ianlancetaylor modified the milestones: Go2, Proposal Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) LanguageChange Suggested changes to the Go language LanguageChangeReview Discussed by language change review committee Proposal
Projects
None yet
Development

No branches or pull requests

5 participants