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

Fix exception throwing inconsistency #17328

Merged
merged 4 commits into from
Jun 25, 2024
Merged

Conversation

psfinaki
Copy link
Member

@psfinaki psfinaki commented Jun 20, 2024

Another thing discovered when reviewing #17277.
AFAIU was not intentional this way, basically copypaste error.

This doesn't seem to break anything.

Copy link
Contributor

github-actions bot commented Jun 20, 2024

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/FSharp.Core docs/release-notes/.FSharp.Core/8.0.400.md

@psfinaki psfinaki marked this pull request as ready for review June 20, 2024 14:14
@psfinaki psfinaki requested a review from a team as a code owner June 20, 2024 14:14
Copy link
Contributor

@brianrourkeboll brianrourkeboll left a comment

Choose a reason for hiding this comment

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

Hmm.

AFAIU was not intentional this way, basically copypaste error.

While ArgumentOutOfRangeException is primarily used to indicate when an index argument is itself out of the range of valid indices, it does also seem to be used in the BCL where the bounds violation is the result of something more derived, e.g., here or here.

So invalidArgOutOfRange, which appears to be used mainly when an argument like count indirectly results in bounds being exceeded, seems like it could reasonably raise ArgumentOutOfRangeException instead of ArgumentException.

This doesn't seem to break anything.

This is not strictly true — there could be code out there like this whose behavior would now be different:

let f g =
    try
        g ()
    with
    | :? ArgumentOutOfRangeException -> [1]
    | :? ArgumentException -> [2]
    | _ -> [3]

// Currently returns [2], but would now return [1].
f (fun () -> [] |> List.skip 1)

@psfinaki
Copy link
Member Author

I agree with the above, I made this change to have a discussion :)

I understand this can change the behavior in theory, I wondered if it breaks something right away.

We can keep things as they are of course, I am really just thinking thinking what to do with this thing in the other PR. My goal is just to minimize the code where it promises one exception and actually throws another one.

@abonie
Copy link
Member

abonie commented Jun 24, 2024

@psfinaki please add release notes and mention there the possibility of breaking change. And if you could add a "Breaking Changes" section to https://github.com/dotnet/fsharp/blob/main/docs/release-notes/.FSharp.Core/8.0.400.md, that would be great.

@psfinaki psfinaki enabled auto-merge (squash) June 25, 2024 10:28
@psfinaki
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants