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

go/ast: Add CallExpr.IsVariadic #17222

Closed
joshlf opened this issue Sep 25, 2016 · 2 comments
Closed

go/ast: Add CallExpr.IsVariadic #17222

joshlf opened this issue Sep 25, 2016 · 2 comments
Assignees

Comments

@joshlf
Copy link

@joshlf joshlf commented Sep 25, 2016

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go1.7.1

Currently, in order to figure out whether a go/ast.CallExpr refers to a variadic function call, the only option is to check whether the Ellipsis field refers to a valid position. In fact, searching for Ellipsis.IsValid reveals 10 locations in the standard library in which this approach is taken to checking whether a function call is variadic. This is a tad verbose, but worse, its meaning is non-obvious to someone not intimately familiar with the go/ast package.

Instead, I propose that it would be nice if there were a convenience method on CallExpr, IsVariadic:

func (x *CallExpr) IsVariadic() bool {
    return x.Ellipsis.IsValid()
}
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 25, 2016

Letting @griesemer decide.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Sep 26, 2016

The original idea behind the AST is to be minimal, in the sense that it only defines the node types, without predicates for each node; mostly because they are not needed. (I agree that this is not true anymore, especially for CommentGroups and Ident, unfortunately).

So, yes, it would be nice if there were a convenience method, but then the question arises, where do we stop? Should there also be a convenience method to test if a FuncType has a "func" or not? And whether a ConstantSpec has a type, etc? It's hard to draw a boundary at that point. Once we have convenience functions, one and the same thing can be computed in two different ways, via the convenience function, or by direct inspection of the node. Which raises yet another question: What's the recommended way? What if code doesn't adhere to it? etc. etc.

The AST does represent the source code almost exactly 1:1, so if there's a "..." in the source, the Ellipsis position is set, otherwise it's not. It does require some knowledge of the syntax, I agree, but that doesn't seem onerous if one is working with the AST. Finally, note that the AST doesn't make a semantic judgement - just because there's a "..." doesn't mean the call is variadic. The "..." here means that the last argument is expected to be a slice that's used with a variadic function, but that may not be the case. So, HasEllipsis() bool would be a better predicate. But there's virtually no difference between HasEllipsis and Ellipsis.IsValid(), so why bother?

I'll say, let's leave this as is. I will make the comment clearer and then close this. Thanks.

@gopherbot gopherbot closed this in 196df6f Sep 26, 2016
@golang golang locked and limited conversation to collaborators Sep 26, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.