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

deprecate { start..finish } { start..step..finish } #17772

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

edgarfgp
Copy link
Contributor

@edgarfgp edgarfgp commented Sep 20, 2024

Description

Implements fsharp/fslang-suggestions#1033

Deprecate { start..finish } and { start..step..finish }

Checklist

  • Test cases added
  • Release notes entry updated

Copy link
Contributor

github-actions bot commented Sep 20, 2024

❗ Release notes required

@edgarfgp,

Caution

No release notes found for the changed paths (see table below).

Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format.

The following format is recommended for this repository:

* <Informative description>. ([PR #XXXXX](https://github.com/dotnet/fsharp/pull/XXXXX))

See examples in the files, listed in the table below or in th full documentation at https://fsharp.github.io/fsharp-compiler-docs/release-notes/About.html.

If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

You can open this PR in browser to add release notes: open in github.dev

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/9.0.200.md No release notes found or release notes format is not correct
LanguageFeatures.fsi docs/release-notes/.Language/preview.md No release notes found or release notes format is not correct

@edgarfgp edgarfgp changed the title Treat { 1..10 } as sequence expression Treat { 1;10 } as sequence expression Sep 20, 2024
@edgarfgp edgarfgp force-pushed the improve-sequence-expr-inference branch from 59c5208 to d643cfb Compare September 20, 2024 21:30
@vzarytovskii
Copy link
Member

I am not 100% sure what exactly was approved in the comment, but I don't immediately see it as "let's allow omitting seq for those cases everywhere. It is taking about when we pass it to collections.

@edgarfgp
Copy link
Contributor Author

I am not 100% sure what exactly was approved in the comment, but I don't immediately see it as "let's allow omitting seq for those cases everywhere. It is taking about when we pass it to collections.

@vzarytovskii what I understood from Don’s comment was that we want to enforce the seq in places where we pass it to collections via a warning or informational warning.

To do this we need to have valid code for { 1;10} to show such a warning right(We do not show warnings on invalid code AFIK) ?

Also note that { 1..10 } is currently valid currently. So { 1;10} arguable should also be.

To summarise my current approach is:

  • Make {1;10} valid code for consistency
  • Add a warning to force the use of seq when passing it to collections.

@vzarytovskii
Copy link
Member

I am not 100% sure what exactly was approved in the comment, but I don't immediately see it as "let's allow omitting seq for those cases everywhere. It is taking about when we pass it to collections.

@vzarytovskii what I understood from Don’s comment was that we want to enforce the seq in places where we pass it to collections via a warning or informational warning.

To do this we need to have valid code for { 1;10} to show such a warning right(We do not show warnings on invalid code AFIK) ?

Also note that { 1..10 } is currently valid currently. So { 1;10} arguable should also be.

To summarise my current approach is:

  • Make {1;10} valid code for consistency

  • Add a warning to force the use of seq when passing it to collections.

Hm, I've read it a bit differently -

That is, if the seq is being immediately consumed eagerly into a collection construction then I'm happy to omit it.

I read it as - when we immediately pass it to collections, then we do not require "seq" (even for now illegal constructs), otherwise (if not passed to collections) we add a warning enforcing (suggesting really) to add "seq" to "{ x .. y }".

But I have problems decoding the comment w.r.t. allowing other constructs to omit "seq" when not passed to collections (when returning or binding for example.

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Sep 21, 2024

But I have problems decoding the comment w.r.t. allowing other constructs to omit "seq" when not passed to collections (when returning or binding for example.

what about let x = { 1..10 } should this now be disallowed by a warning. I’m trying to understand the rationale about the existing inconsistency between both cases in bindings ?

@vzarytovskii
Copy link
Member

But I have problems decoding the comment w.r.t. allowing other constructs to omit "seq" when not passed to collections (when returning or binding for example.

what about let x = { 1..10 } should this now be disallowed by a warning. I’m trying to understand the rationale about the existing inconsistency between both cases in bindings ?

That's what I understood. Show a diagnostic suggesting adding "seq" if it's not passed to collection. And allow other constructs only when passed to collections.

I'd wait for other folks to read it through, I might be misunderstanding things.

@edgarfgp edgarfgp changed the title Treat { 1;10 } as sequence expression deprecate { start..finish } and { start;finish } using a warning Sep 21, 2024
@edgarfgp edgarfgp changed the title deprecate { start..finish } and { start;finish } using a warning deprecate { start..finish } { start..step..finish } Sep 21, 2024
@edgarfgp edgarfgp force-pushed the improve-sequence-expr-inference branch 2 times, most recently from c05956b to 9c05432 Compare September 22, 2024 12:40
@edgarfgp edgarfgp force-pushed the improve-sequence-expr-inference branch from 9c05432 to 47b4ef0 Compare September 22, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: New
Development

Successfully merging this pull request may close these issues.

2 participants