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/godoc: unexported fields are displayed in exported variable initializers #22803

Closed
ghost opened this issue Nov 19, 2017 · 9 comments
Closed
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@ghost
Copy link

ghost commented Nov 19, 2017

What did you do?

https://godoc.org/github.com/opennota/rwc

What did you expect to see?

godoc doesn't show unexported fields.

What did you see instead?

godoc for the DefaultConstructor variable shows unexported fields:

var DefaultConstructor = Constructor{
    ng4: [32768]uint32{ /* 32768 elements not displayed */

    },
    ng3: [1024]uint32{ /* 1024 elements not displayed */

    },
    ng3beg: [1024]uint32{ /* 1024 elements not displayed */

    },
    ng3end: [1024]uint32{ /* 1024 elements not displayed */

    },
    ng2: [32]uint32{
        18874896, 134742016, 134234113, 16424, 536887585, 1073748512,
        289, 16417, 2101380, 0, 679937, 2416460032, 134217729, 540961,
        2372130, 16385, 1, 1610629156, 1208500513, 3280960, 524289, 16384,
        0, 1056, 1, 0, 0, 4096, 0, 2097664, 8, 34320,
    },
    ng1: 2148156709,
}
@bradfitz bradfitz added this to the Unplanned milestone Nov 20, 2017
@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 20, 2017
@bradfitz bradfitz changed the title godoc: unexported fields are displayed anyway x/tools/godoc: unexported fields are displayed in exported variable initializers Nov 20, 2017
@bradfitz
Copy link
Contributor

@robpike, @griesemer, any opinions? Is this expected?

@griesemer
Copy link
Contributor

griesemer commented Nov 20, 2017

I think this is expected (I don't think we have special code to filter those fields, but I haven't checked); when there's an initialization expression we simply print that expression. That is not to say that this is correct (or wrong for that matter).

@robpike
Copy link
Contributor

robpike commented Nov 20, 2017

It doesn't bother me much, although I can understand why some would be distracted.

@rsc
Copy link
Contributor

rsc commented Nov 27, 2017

The proposal-review group is fine with unexported field initializations being replaced with "..." just as for unexported fields in type definitions. We don't anticipate working on it ourselves.

@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 27, 2017
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 27, 2017
@agnivade
Copy link
Contributor

agnivade commented Apr 6, 2018

I am thinking of working on this. This is my first time tinkering with ASTs, so I would need a little guidance.

Just to be clear, the fix needs to be done in the go/ast package which actually generates printed representation of the AST, and not inside x/tools/godoc.

Initially, I started making changes in the go/printer package, but then later realized that the filtering of fields and idents is done from inside the go/ast package. So, I have started work there. I have a working version ready which needs some finishing touches. Here is what I have done so far:

  • Added an Incomplete field inside CompositeLit similar to StructType to signal the printer package that fields are missing.
  • Added a filterExpressionList function in ast/filter.go which goes through a slice of []Expr checking for KeyValue expressions inside CompositeLiterals. If it finds that the Ident.Name of the Key is not exported, it filters that element from the slice along with setting the Incomplete field to true.
  • Pending is to take this information in the go/printer package and either print a ... or the usual // contains filtered or unexported fields.

Does this look like the correct approach ?

@griesemer
Copy link
Contributor

@agnivade Your suggestions sound reasonable and match what we do for struct fields. Please send CLs directly to me for review. Thanks.

@agnivade
Copy link
Contributor

agnivade commented Apr 9, 2018

Perfect. Thanks !

@gopherbot
Copy link

Change https://golang.org/cl/106395 mentions this issue: go/ast: hide unexported fields in composite literals

@agnivade
Copy link
Contributor

Attaching a screenshot of how it looks now -

unexport

@agnivade agnivade self-assigned this Apr 13, 2018
@golang golang locked and limited conversation to collaborators May 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants