Implement struct slop detection rules

[cuonglm]

Using go/types.Sizes interface.

[odeke-em]

Thank you for this change @cuonglm! So some concerns:
a) We actually should generate for them the optimal organization and show them which fields to re-arrange instead of making them fiddle and do the Maths themselves -- this is error prone and tedious for users
b) We need to implement this with the x/analysis framework, which would involve a slightly different organization than Matthew's code
c) Given that this code will be owned by Orijtech, I am worried about code copying binding us to Matthew's code. Let's implement this afresh, there is a straightforward approach to it

[odeke-em]

At least from the expected string I didn’t see a change in the struct fields except it saying was 24 can be 16.

…

On Mon, Sep 21, 2020 at 1:34 AM Cuong Manh Le ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In structslop.go <#5 (comment)>: > return } - pass.Reportf(s.Pos(), "not implemented") + + if styp.NumFields() < 2 { + return + } + + if r := malign(styp); r.Slop() { + pass.Report(analysis.Diagnostic{ + Pos: n.Pos(), + End: n.End(), + Message: fmt.Sprintf("%v has size %d, could be %d", styp, r.oldSize, r.newSize), + SuggestedFixes: []analysis.SuggestedFix{{ + Message: fmt.Sprintf("Rearrange struct fields: %v", r.suggestedStruct), @odeke-em <https://github.com/odeke-em> would you mind elaborating? This suggested fix actually generate new optimal struct for user. If you build the binary and run on testdata with -fix, you will see the slop struct was modified — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFL3V7VU4CBCKZ7ZCPME63SG4F23ANCNFSM4RUCOBFQ> .

[cuonglm]

At least from the expected string I didn’t see a change in the struct fields except it saying was 24 can be 16.

This is how analysis frame work works. The message only shows what’s wrong. The suggested fixed will be applied to the source when you run with -fix flag.

I think a flag to actually display the fix to user will be added soon.

[odeke-em]

I think a flag to actually display the fix to user will be added soon.

And that’s what this task is about. I think we need a from the ground up approach where we build and display the optimal struct, not just what maligned displays as a warning.

[cuonglm]

And that’s what this task is about. I think we need a from the ground up approach where we build and display the optimal struct, not just what maligned displays as a warning.

Do you mean we will make a CL to go/analysis to do that?

Note that this PR built the optimal struct already (look at suggestedStruct field in result struct)

[cuonglm]

@odeke-em I updated to show suggested fix to user, I think the only thing left is porting the code from maligned to our own.

[cuonglm]

@odeke-em PTAL, latest commit should address all your concern.

[odeke-em]

Nice work @cuonglm, this is much better and built from the ground up! Just some minor nits.
In regards to more test cases, let's add

type s1 struct {
	b  bool
	i1 int
	i2 int
	a3 [3]bool
	_  [0]func()
}

Let's also a TODO to recognize the special case of

_ [0]func()

if at the front and it shouldn't be moved.

[odeke-em]

Also please update the commit message as this code is anew and doesn't refer to nor depend on maligned.

[cuonglm]

Also please update the commit message as this code is anew and doesn't refer to nor depend on maligned.

Will do it once the PR is ready to merge (squashing before merge).

[cuonglm]

Let's also a TODO to recognize the special case of

Could you elaborate this? Do you mean if the _ [0]func() is at the front, we shouldn't move it eve if the moving can make struct size better?

[odeke-em]

Do you mean if the _ [0]func() is at the front, we shouldn't move it eve if the moving can make struct size better?

Exactly that. This is because that’s an intentional change to make the struct incomparable and we want to keep that. All other fields can be moved around except for that one iff it exists as the first element.

[cuonglm]

Do you mean if the _ [0]func() is at the front, we shouldn't move it eve if the moving can make struct size better?

Exactly that. This is because that’s an intentional change to make the struct incomparable and we want to keep that. All other fields can be moved around except for that one iff it exists as the first element.

I don't think the position of _ [0]func() does matter. It can be appear anywhere in the struct fields, and still make the struct incomparable.

https://play.golang.org/p/iG8z94Mp-RD

[odeke-em]

For it not to consume space it has to be the first element :) that’s the difference

…

On Mon, Sep 21, 2020 at 8:06 PM Cuong Manh Le ***@***.***> wrote: Do you mean if the _ [0]func() is at the front, we shouldn't move it eve if the moving can make struct size better? Exactly that. This is because that’s an intentional change to make the struct incomparable and we want to keep that. All other fields can be moved around except for that one iff it exists as the first element. I don't think the position of _ [0]func() does matter. It can be appear anywhere in the struct fields, and still make the struct incomparable. https://play.golang.org/p/iG8z94Mp-RD — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFL3V3X5RETZCQUYIH3VNDSHAIDJANCNFSM4RUCOBFQ> .

[cuonglm]

For it not to consume space it has to be the first element :) that’s the difference
…
On Mon, Sep 21, 2020 at 8:06 PM Cuong Manh Le @.***> wrote: Do you mean if the _ [0]func() is at the front, we shouldn't move it eve if the moving can make struct size better? Exactly that. This is because that’s an intentional change to make the struct incomparable and we want to keep that. All other fields can be moved around except for that one iff it exists as the first element. I don't think the position of _ [0]func() does matter. It can be appear anywhere in the struct fields, and still make the struct incomparable. https://play.golang.org/p/iG8z94Mp-RD — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABFL3V3X5RETZCQUYIH3VNDSHAIDJANCNFSM4RUCOBFQ .

Ah, that makes sense, TIL.

Do you have any reference for this? (or I will take a look at the compiler when I have time 👍 )

[odeke-em]

I don’t think it is written down, derived from playing around with the compiler and runtime. You’ll also need this trivia when we write the blogpost for this work ;) On Mon, Sep 21, 2020 at 8:09 PM Cuong Manh Le <notifications@github.com> wrote:

…

For it not to consume space it has to be the first element :) that’s the difference … <#m_-6573522252259619745_> On Mon, Sep 21, 2020 at 8:06 PM Cuong Manh Le *@*.***> wrote: Do you mean if the _ [0]func() is at the front, we shouldn't move it eve if the moving can make struct size better? Exactly that. This is because that’s an intentional change to make the struct incomparable and we want to keep that. All other fields can be moved around except for that one iff it exists as the first element. I don't think the position of _ [0]func() does matter. It can be appear anywhere in the struct fields, and still make the struct incomparable. https://play.golang.org/p/iG8z94Mp-RD — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5 (comment) <#5 (comment)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABFL3V3X5RETZCQUYIH3VNDSHAIDJANCNFSM4RUCOBFQ . Ah, that makes sense, TIL. Do you have any reference for this? (or I will take a look at the compiler when I have time 👍 ) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFL3V4XW7YYTLN54Q7VAJ3SHAIOHANCNFSM4RUCOBFQ> .