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

spec: for-range iterator yield function requires bool result - consider generalizing to any boolean type #71164

Open
griesemer opened this issue Jan 7, 2025 · 5 comments
Assignees
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. LanguageChange Suggested changes to the Go language NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@griesemer
Copy link
Contributor

Per the spec, a "for-range" statement using an iterator requires the iterator's yield function to return bool not an arbitrary (user-defined) boolean:

Range expression                                       1st value                2nd value

array or slice      a  [n]E, *[n]E, or []E             index    i  int          a[i]       E
string              s  string type                     index    i  int          see below  rune
map                 m  map[K]V                         key      k  K            m[k]       V
channel             c  chan E, <-chan E                element  e  E
integer value       n  integer type, or untyped int    value    i  see below
function, 0 values  f  func(func() bool)
function, 1 value   f  func(func(V) bool)              value    v  V
function, 2 values  f  func(func(K, V) bool)           key      k  K            v          V

We should generalize this to any boolean type, similarly to how we allow any string type (not just string) when we range of strings.

Note that the original implementation of the type-checker accepted any boolean type, but the compiler's front-end had a problem with it (#71131). The (temporary) fix for that issue was to adjust the type-checker to match the spec literally. This avoided a compiler panic.

We should change the spec to reflect the original intent, and then revert the fix for #71131.

@griesemer griesemer added LanguageChange Suggested changes to the Go language NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 7, 2025
@griesemer griesemer added this to the Backlog milestone Jan 7, 2025
@griesemer griesemer self-assigned this Jan 7, 2025
@griesemer griesemer modified the milestones: Backlog, Go1.25 Jan 7, 2025
@griesemer griesemer added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Jan 7, 2025
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/640599 mentions this issue: go/types, types2: require iterator yield to return bool (work-around)

@gabyhelp
Copy link

gabyhelp commented Jan 7, 2025

Related Issues

Related Code Changes

Related Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@dolmen
Copy link
Contributor

dolmen commented Jan 8, 2025

We should generalize this to any boolean type, similarly to how we allow any string type (not just string) when we range of strings.

I'm not convinced by this. Generalizing implies a incompatible change that will affect introspecting at runtime (using reflect) functions that might be iterators.

So could you develop why allowing any bool derived type would be better? What use case does that open?

@griesemer
Copy link
Contributor Author

The implementation (type checkers, at least) were written from the start to accept any boolean type. The compiler front-end has a bug that causes it to panic when a user-defined boolean is used (#71131). Fixing that bug at this stage of the 1.24 release cycle is risky, so we opted for reporting an error in the type-checker, which is very safe, and which avoids a compiler panic in favor of a very clear error message. We should really fix the compiler front-end (early 1.25 cycle) and then we can also relax this restriction again. I don't know that there are particular issues with reflect at this point since such code didn't work anyway.

@seankhliao
Copy link
Member

Is allowing this really good for the ecosystem? these iterators wouldn't be assignable to iter.Seq / iter.Seq2 and would not be composable with any of the iterator helpers out there.
It seems strange to allow for covariant result types in this specific corner of the language when its disallowed elsewhere with a faq entry for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. LanguageChange Suggested changes to the Go language NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

6 participants