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/internal/ir: OCONVNOP is documented as having no effect, but it might change the type #71841

Open
mateusz834 opened this issue Feb 19, 2025 · 3 comments
Labels
Documentation Issues describing a change to documentation. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@mateusz834
Copy link
Member

I am unsure whether this is a bug or not, but the current docs of OCONVNOP is as follows:

OCONVNOP // Type(X) (type conversion, no effect)

Note the "no effect", but actually it can change the type:

type Impl struct{}
type implWrapper Impl

func _() {
	i := &Impl{}
        _ = (*implWrapper)(i) // OCONVNOP
}

Is this the expected behavior? If so i think that the docs should be updated, as it might be confusing (as it is a NOP).

CC @golang/compiler

@gopherbot gopherbot added the Documentation Issues describing a change to documentation. label Feb 19, 2025
@mateusz834 mateusz834 added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 19, 2025
@randall77
Copy link
Contributor

The point of OCONVNOP is to change the type.
Like this:

type T int64

func f(x int64) T {
	return T(x)
}

The NOP means that no code is needed to do it.

Sure, we can update the docs to be more clear about it.

@mateusz834
Copy link
Member Author

Thanks, my initial thought was only cases like: int(int(0)).

That makes me also think whether ir.StaticValue should check for such cases, currently it follows such CONVs:

func StaticValue(n Node) Node {
for {
switch n1 := n.(type) {
case *ConvExpr:
if n1.Op() == OCONVNOP {
n = n1.X
continue
}

At least it does not seem to cause any issues for the devirtualizer, as there also is a OCONVINTER, which is not followed here.

@gabyhelp
Copy link

Related Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@gabyhelp gabyhelp added the LanguageProposal Issues describing a requested change to the Go language specification. label Feb 19, 2025
@mateusz834 mateusz834 removed the LanguageProposal Issues describing a requested change to the Go language specification. label Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues describing a change to documentation. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants