primitives should strongly declare their types except in default cases

[fkautz]

We're running into scenarios where the emitted code from valast is not valid due to missing types. We traced this down to the generated string representation not adding type information. This patch causes most emitted types to perform a typecast to seed the information properly to the compiler.

See #15 for details. Fixes #15

Signed-off-by: Frederick F. Kautz IV fkautz@alumni.cmu.edu

[slimsag]

Thank you for the PR! Unfortunately I don't think this is the right fix in the current form.

In this if case, opt.Unqualify signals:

	// Unqualify, if true, indicates that types should be unqualified. e.g.:
	//
	// 	int(8)           -> 8
	// 	Bar{}            -> Bar{}
	// 	string("foobar") -> "foobar"
	//
	// This is set to true automatically when operating within a context where type qualification
	// is definitively not needed, e.g. when producing values for a struct or map.
	Unqualify bool

That is, if opt.Unqualify is true (this if case is executing) then the value should not have a qualified type.

In the case of #15, it seems like the issue is that we are producing:

valast.str{ExpirationSeconds: valast.Addr(3607).(*int64)}

instead of the expected:

valast.str{ExpirationSeconds: valast.Addr(int64(3607)).(*int64)}

I think that what this means is that we're not correctly setting Unqualify to false (so the int64() is added) when we emit the expression for valast.Addr(expr) - the fix will need to be located somewhere around that code.

We emit valast.Addr here:

valast/valast.go

Lines 488 to 501 in c24911a

	return Result{
	AST: &ast.TypeAssertExpr{
	X: &ast.CallExpr{
	Fun: &ast.SelectorExpr{
	X: ast.NewIdent("valast"),
	Sel: ast.NewIdent("Addr"),
	},
	Args: []ast.Expr{elem.AST},
	},
	Type: ptrType.AST,
	},
	RequiresUnexported: ptrType.RequiresUnexported || elem.RequiresUnexported,
	OmittedUnexported: elem.OmittedUnexported,
	}, nil

The expression is here:

valast/valast.go

Line 495 in c24911a

Args: []ast.Expr{elem.AST},

I think maybe we just need to move this code into this conditional and set opt.Unqualify = false before invoking it - so that int64() gets added.

[fkautz]

Thank you for the detailed response. The idea was to write some code to spur this kind of feedback. You did not disappoint! 🥇

I'll give this a shot tomorrow and resubmit.

[fkautz]

More information on this, the following code results in a build failure:

	type str struct {
		ExpirationSeconds *int32
	}

	broken := str{ExpirationSeconds: valast.Addr(3607).(*int32)}
	fmt.Println(broken)

In short, Unqualified in a struct yields code that is invalid.

panic: interface conversion: interface {} is *int, not *int32

What are your thoughts on always forcing qualified types when working with pointers?

[slimsag]

I would expect your snippet above to be:

	type str struct {
		ExpirationSeconds *int32
	}

-	broken := str{ExpirationSeconds: valast.Addr(3607).(*int32)}
+	broken := str{ExpirationSeconds: valast.Addr(int32(3607)).(*int32)}
	fmt.Println(broken)

Then you would not get such a panic, as the type would be qualified. This is only needed when the input value is a integer literal and the type assertion is not int.

I'm happy to look into fixing this for you if you want to convert this into a bug report BTW

[fkautz]

A bug exists for this issue at #15.

I would greatly appreciate any help here. I'm not partial/biased to this code or the time I put in, so if you have a solution in mind, I will not feel bad if you end up closing this and replacing it with your own implementation. :)

I tried a workaround by setting Unqualified to false, but it still emits unqualified code. I'll open another bug for that soon.

[slimsag]

This, for example, is an update I wouldn't expect to see: [1]float32{1} is completely valid and so the type qualifier float32(1) shouldn't be needed.

[krisnova]

Pointing back to krisnova/naml#62

[fkautz]

Just a heads up, I'm travelling so I'm going to be a bit slower on this. Haven't forgotten about it. :)

[fkautz]

Just want to point out there is a new comment inline above. GitHub UI makes this easy to miss. Thanks for your help!

#16 (comment)

[slimsag]

@fkautz Thanks a ton again for reporting this! Just fixed this in #19 and released v1.4.1, let me know if you run into any other trouble :)

[fkautz]

Fantastic, thank you very much!

On Sat, Nov 20, 2021 at 4:54 PM Stephen Gutekanst ***@***.***> wrote: @fkautz <https://github.com/fkautz> Thanks a ton again for reporting this! Just fixed this in #19 <#19> and released v1.4.1, let me know if you run into any other trouble :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABBEGWYEFSHTX7WBY7VJ5LUNAYNDANCNFSM5BYETGUA> .

-- -- Frederick F. Kautz IV