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

x/tools/internal/refactor/inline: some implicit conversions are undetectable using go/types #63193

Closed
adonovan opened this issue Sep 24, 2023 · 5 comments
Assignees
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@adonovan
Copy link
Member

Consider func f(x int16) { y := x; _ = (*int16)(&y) }. If inlining a call f(1) results in parameter substitution then the output is something like { y := 1; _ = (*int16)(&y) }, which doesn't compile because the type of y has changed from int16 to int.

The inliner already has a check in place so that nontrivial implicit conversions prevent substitution, but the check does not fire in this case because, according to the type checker, the type of the constant 1 in f(1) is int16, not untyped int. (I'm pretty sure I requested this helpful feature years ago, sigh.) Once the program has been transformed, the statement y := 1 has forgotten all about int16.

Unfortunately I think we will need an additional analysis of the argument expression to ascertain whether its type is a purely "bottom-up" property of the syntax or depends on a "top-down" property of the context. If CheckExpr were less inflexible we might be able to invoke it on the argument expression (effectively, with no context) to see what type it gives.

@griesemer @findleyr

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 24, 2023
@gopherbot gopherbot added this to the Unreleased milestone Sep 24, 2023
@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 25, 2023
@findleyr
Copy link
Member

Yes, this is an unfortunate loss of information (in retrospect, a nice goal for go/types would be to lose no information, but it took me a long time to realize that).

Supposing we wanted to fix this, I can think of two approaches:

  1. Add another map to types.Info.
  2. Add a new CheckExpr-like API that works the way we want.

I feel like I'd rather invest in (2), though I can't say for certain that it is feasible. What do others think?

@adonovan adonovan self-assigned this Sep 25, 2023
@adonovan
Copy link
Member Author

If #62664 (comment) we might be able to invoke it on the argument expression (effectively, with no context) to see what type it gives.

I was unnecessarily critical of CheckExpr. It solves this problem neatly.

@timothy-king
Copy link
Contributor

My worry about CheckExpr with no context is more complex expressions with constants:

func f(x int16) { y := x; _ = (*int16)(&y) }
const c = 1
f(c+2) // inline

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/530975 mentions this issue: internal/refactor/inline: insert conversions during substitition

@adonovan
Copy link
Member Author

My worry about CheckExpr with no context is more complex expressions with constants:
func f(x int16) { y := x; _ = (*int16)(&y) }
const c = 1
f(c+2) // inline

With my pending CL 530975, this example inlines to:

            	const c = 1
            	{
            		y := int16(c + 2)
            		_ = (*int16)(&y)
            	}

which is correct.

@golang golang locked and limited conversation to collaborators Sep 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

5 participants