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: Expose whether functions are variadic in their pp() output #17360

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

smowton
Copy link
Contributor

@smowton smowton commented Sep 3, 2024

This avoids our having types that are distinct both in the database and are distinguishable in QL, but which pretty-print identically.

@smowton smowton requested a review from a team as a code owner September 3, 2024 11:53
@github-actions github-actions bot added the Go label Sep 3, 2024
@smowton smowton added the no-change-note-required This PR does not need a change note label Sep 3, 2024
Copy link
Member

@mbg mbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, seems like a sensible change. One suggestion for improvement, if you are so inclined: perhaps it would make sense to put the [variadic] inside the func( ... ) part, rather than after the return types?

Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Why not copy how being variadic is normally shown in go signatures, i.e. func(...interface { }) int, error? It's a bit more involved to remove the [] from the last parameter type, but it would be easier for everyone to understand (who is familiar with go).

Comment on lines +918 to +923
exists(string suffix | (if this.isVariadic() then suffix = " [variadic]" else suffix = "") |
result =
"func(" + concat(int i, Type tp | tp = this.getParameterType(i) | tp.pp(), ", " order by i) +
") " + concat(int i, Type tp | tp = this.getResultType(i) | tp.pp(), ", " order by i) +
suffix
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
exists(string suffix | (if this.isVariadic() then suffix = " [variadic]" else suffix = "") |
result =
"func(" + concat(int i, Type tp | tp = this.getParameterType(i) | tp.pp(), ", " order by i) +
") " + concat(int i, Type tp | tp = this.getResultType(i) | tp.pp(), ", " order by i) +
suffix
)
result =
"func(" +
concat(int i, Type tp, string prefix |
if i = this.getNumParameter() - 1 and this.isVariadic()
then
tp = this.getParameterType(i).(SliceType).getElementType() and
prefix = "..."
else (
tp = this.getParameterType(i) and
prefix = ""
)
|
prefix + tp.pp(), ", " order by i
) + ") " + concat(int i, Type tp | tp = this.getResultType(i) | tp.pp(), ", " order by i)

This implements printing "...interface{ }" as I suggested in my previous review.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you could also keep the [variadic] to highlight variadic functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Go no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants