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: remove function definitions w/ invalid return types #599

Merged
merged 2 commits into from
Feb 22, 2024

Conversation

vbarua
Copy link
Member

@vbarua vbarua commented Feb 8, 2024

T? and T|? are not valid return types

T|? is equivalent to setting the nullability to MIRROR
T&? is not currently expressible via nullability

BREAKING CHANGE: least_skip_null and greatest_skip_null functions have been removed

T? and T|? are not valid return types

T|? is equivalent to setting the nullability to MIRROR
T&? is not currently expressible via nullability

BREAKING CHANGE: least_skip_null and greatest_skip_null functions have been removed
@richtia
Copy link
Member

richtia commented Feb 8, 2024

Here's the PR that introduced those for reference: #247

@vbarua
Copy link
Member Author

vbarua commented Feb 8, 2024

@richtia thanks for linking that 🙇

Additional context from @westonpace in Slack

No. Let's back those out for now and create a todo. We had some discussion on this topic in the community meeting. The conclusion was that we do not want to add new syntax to the standard return types.
Instead, those should use the same "return type is a small block of code that you have to evaluate" approach that is used for decimal arithemtic, e.g.

        return: |-
          init_scale = max(S1,S2)
          init_prec = init_scale + max(P1 - S1, P2 - S2) + 1
          min_scale = min(init_scale, 6)
          delta = init_prec - 38
          prec = min(init_prec, 38)
          scale_after_borrow = max(init_scale - delta, min_scale)
          scale = init_prec > 38 ? scale_after_borrow : init_scale
          DECIMAL<prec, scale>

Part of the discussion in Slack was also around adding a new nullability type like INTERSECTION for returns non-null if any of the input arguments is non-null as comes up in a number of places (i.e. coalesce).

EpsilonPrime
EpsilonPrime previously approved these changes Feb 9, 2024
westonpace
westonpace previously approved these changes Feb 9, 2024
Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

I approve if we want to proceed with this. However, would it be better to keep the methods around and just change the return type to T? Is "slightly inaccurate but usable" better than "accurate but not usable"?

T? and T|? are not valid return types

T|? is equivalent to setting the nullability to MIRROR
T&? is not currently expressible via nullability

BREAKING CHANGE: least_skip_null and greatest_skip_null functions have
updated return types
@vbarua vbarua dismissed stale reviews from westonpace and EpsilonPrime via e183986 February 9, 2024 23:51
@vbarua
Copy link
Member Author

vbarua commented Feb 9, 2024

However, would it be better to keep the methods around and just change the return type to T? Is "slightly inaccurate but usable" better than "accurate but not usable"?

@westonpace I think it makes sense to keep them available so that they can be used. In this case, using nullability MIRROR is weaker than the actual nullability desired so it seems safe.

Made a change to bring them back in, update the return types and include a note about it.

I've created #601 as an issue to track this new desired nullability as well.

@westonpace
Copy link
Member

If @EpsilonPrime is still good with the updated version I think we are good to go

@vbarua vbarua requested a review from EpsilonPrime February 21, 2024 21:08
Copy link
Member

@bvolpato bvolpato left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks.

Note, this part of the description is not true anymore:

BREAKING CHANGE: least_skip_null and greatest_skip_null functions have been removed

@vbarua
Copy link
Member Author

vbarua commented Feb 22, 2024

Will merge this as this has 3 SMC votes (including myself)

@vbarua vbarua merged commit a3b1f32 into main Feb 22, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants