Git Product home page Git Product logo

Comments (19)

CyrusNajmabadi avatar CyrusNajmabadi commented on July 17, 2024 4

I eprsonally disagree. Using throw is normal and idiomatic. If there is something common enough it could benefit from a lib, then put in a lib. But that seems like the very narrow case. The runtime seems to only have a tiny handful of these, just for the cases where they cared about perf enough for thsi to be relevant. I would not extend that out to any sort of pattern beyond that.

from csharplang.

hez2010 avatar hez2010 commented on July 17, 2024 2
x is T and P type and P check x as T and P cast to T? (or a more specific applicable type from P), ensuring P, or null on failure

This doesn't follow today's is semantic where it allows you casting x to both T and P:

if (x is T t and P p) {
  // use t and p here
}

Today we are not allowed to do

if (x is (T and P) tp) // error, waiting for union/intersection types coming in future

Allowing as T and P today would block the future of union/intersection types in C#, as ideally x as T and P should result in type T and P (T & P).

from csharplang.

julealgon avatar julealgon commented on July 17, 2024 1

How would you reconcile this with all the guard methods that were recently introduced, such as ArgumentOutOfRangeException.ThrowIfNegative and such?

The reason they exist is to avoid the whole nameof(param) thing (which by itself is error prone), and to centralize the various message variations (there are many for ArgumentOutOfRangeException).

By using the old ?? throw new... throw expression syntax, you just lose all the benefits of the native guard methods.

I don't disagree with what you are proposing in essence (I've built something similar using .As extension methods actually before), but at the same time I don't want to lose the great benefits that the guard methods provide.

I've actually completely replaced all of these in our codebase with the guard methods recently. For example:

this.user = user ?? throw new ArgumentNullException(nameof(user));

With:

ArgumentNullException.ThrowIfNull(user);
this.user = user;

I don't think approving this with the intent of using it for argument validation is a great idea without also reconsidering all the guard methods: an ideal solution would handle both things.

The ultimate approach to me would be to properly add contracts to the language and allow this (which has been discussed a lot in multiple issues by now):

public MyClass(string user, int value)
    where user is not null
    where value is >= 0 and < 100
{
    this.user = user;
    this.value = value;
}

from csharplang.

TahirAhmadov avatar TahirAhmadov commented on July 17, 2024 1

I think these type unions and intersections should be prohibited in the proposed as syntax for now. It's always easier to prohibit now, then allow later once proper design is done for it.

from csharplang.

IS4Code avatar IS4Code commented on July 17, 2024 1

@bbarry Yes, #8207 is what this started as.

from csharplang.

hez2010 avatar hez2010 commented on July 17, 2024 1

@Thaina I don't like it either, but that is how patterns work in general ‒ x is >= 0 checks for int.

It's using targeted type. If x is float it will match against float as well.

from csharplang.

CyrusNajmabadi avatar CyrusNajmabadi commented on July 17, 2024

By using the old ?? throw new... throw expression syntax, you just lose all the benefits of the native guard methods.

That seems like a runtime issue. This pattern woudl be fairly trivial/idiomatic. So it could be detected and converted to the same IL that would be emitted for calling into the helpers.

from csharplang.

IS4Code avatar IS4Code commented on July 17, 2024

It's a good point that there are these methods to use, but there also situations where you might not want to produce an exception, or the existing guard methods don't cover all patterns you might want.

There is also a potential of another neat way of parameters checking, using the proposed syntax:

this.x = ArgumentOutOfRangeException.ThrowIfUnmatched(x as >= 0 and <= 255);

from csharplang.

julealgon avatar julealgon commented on July 17, 2024

By using the old ?? throw new... throw expression syntax, you just lose all the benefits of the native guard methods.

That seems like a runtime issue. This pattern woudl be fairly trivial/idiomatic. So it could be detected and converted to the same IL that would be emitted for calling into the helpers.

I don't follow you here @CyrusNajmabadi . How would code like:

this.b = b as >= 0 ?? throw new ArgumentOutOfRangeException(nameof(b));

Avoid having to pass in the explicit exception type, the nameof(b) and the specific error message for the >= validation (all of which are benefits of using the guard methods)?

Or perhaps you are suggesting something like this (which would be very interesting if possible)?

this.b = b as >= 0 ?? throw;

In this case, the compiler could see the various trivial comparisons and pick the correct exception type and message. Is this what you are alluding to?

from csharplang.

CyrusNajmabadi avatar CyrusNajmabadi commented on July 17, 2024

By using the old ?? throw new... throw expression syntax, you just lose all the benefits of the native guard methods.

That presumes that the specific corresponding native guard method exists. And, if they do, that means they have a specific code pattern in them. For example if arg is null throw new ArgNullException(...).

I'm saying, if your callsite has effective the identical pattern that calling the guard method would emit, then the runtime can/should treat them uniformly. There should be no penalization that you happened to call one vs the other.

from csharplang.

julealgon avatar julealgon commented on July 17, 2024

By using the old ?? throw new... throw expression syntax, you just lose all the benefits of the native guard methods.

That presumes that the specific corresponding native guard method exists. And, if they do, that means they have a specific code pattern in them. For example if arg is null throw new ArgNullException(...).

I'm saying, if your callsite has effective the identical pattern that calling the guard method would emit, then the runtime can/should treat them uniformly. There should be no penalization that you happened to call one vs the other.

Oh I see what you mean now @CyrusNajmabadi , but that was not my point. I wasn't saying one would miss "functional" benefits by using a different syntax, but maintenance-related ones.

For example, having to provide the nameof(argument) to the exception is something that is not needed when using the guard methods as they rely on [CallerMemberExpression] to automatically grab the argument name. Not having to provide that is good, as it reduces the chance of mistakes happening (using incorrect values, or introducing a typo when using raw strings etc).

Similarly, each guard method has a built-in message for the ArgumentException and ArgumentOutOfRangeException types, based on the method that is being called. By using those exceptions manually, one needs to then provide a message on every call site to get "equivalent" behavior as the guard methods. Maintaining those messages is also an additional burden, and another source for mistakes to happen.

And lastly, the guard methods also take care of choosing the more appropriate exception type and message based on the underlying checks being done. For example, using ArgumentException.ThrowIfNullOrWhiteSpace will throw 2 distinct exceptions each with their own bespoke message, depending on whether the value passed in is null (ArgumentNullException), or empty, whitespace, etc (ArgumentException + custom message).

This kind of validation is harder to capture using a single expression, especially if using the syntax proposed here, as there is no way to throw different exceptions using a single throw expression after a coalescing operator. So one would need to go down into a switch statement instead, which brings its own issues such as exhaustiveness checks.

My overall point is that any new feature that influences how argument validation is done should seriously consider how the existing guard methods will play with them, even if the answer is to deprecate the guard methods altogether.

I would love to have something like I mentioned above though, which would be a combination of both worlds:

this.b = b as >= 0 ?? throw;

If the compiler can understand the validation pattern being applied, it would pick the correct exception and message to throw automatically. If it cannot understand it, it would signal this with a compilation error.

The only caveat with an approach like this of course is how to make it extensible (and if that's even desirable in the first place). This would be a bit less complex (and less flexible) than full-blown contracts, but still much better than what we currently have.

from csharplang.

HaloFour avatar HaloFour commented on July 17, 2024

Given this feature is not specific to argument validation I don't think it makes sense to try to bake that much "magic" into the language specification for this feature.

from csharplang.

julealgon avatar julealgon commented on July 17, 2024

Given this feature is not specific to argument validation I don't think it makes sense to try to bake that much "magic" into the language specification for this feature.

I don't disagree (what I said above would likely become a separate proposal), but at the same time, the main arguments presented in favor of this proposal here are tied directly to argument validation.

I think they should be considered in conjunction to avoid disrupting existing argument validation patterns and creating clashing standards.

For a very long time, argument validation wasn't strongly standardized. With the introduction of the native guard methods, this has now changed, as most common validations are covered there. With the introduction of extension types in .NET9, it will also be possible to introduce extra guard methods using the exact same pattern as the native ones.

Using this as extended proposal for going back to manual throw expressions would be a regression on that effort IMHO.

from csharplang.

IS4Code avatar IS4Code commented on July 17, 2024

@hez2010 Good point! It would be better to disable P from changing the type of the result in that case.

from csharplang.

IS4Code avatar IS4Code commented on July 17, 2024

Actually, to provide more thoughts, the reasoning behind as T and P was intended primarily to make something like as IComparable and > 10 turn into int? (since > 10 in this situation is an implicit int pattern), i.e. the first type should not determine the result alone.

To refine on this, I think it is sufficient if the pattern is restricted (at the moment) to always result in an identifiable type. Practically, this means that as T1 and T2 is fine only if T1T2 or T2T1, and T1 or T2 is fine only if T1 = T2 (so it is pretty much useless, but is allows it too, so why not). In the future, the other cases could be turned into T1 & T2 or T1 | T2.

from csharplang.

julealgon avatar julealgon commented on July 17, 2024

It's always easier to prohibit now, then allow later once proper design is done for it.

Not only that but it also allows for faster development of the rest of the design and gets some useful additions out of the door without having to develop (and especially discuss/define) the whole thing completely which as we know can take years sometimes.

I particularly like when it is possible to implement something incrementally because of that benefit. It is basically what happened (and is still happening) with pattern matching.

from csharplang.

bbarry avatar bbarry commented on July 17, 2024

Is there a discussion open for this item? I feel like most of the comments here would benefit from the formatting and associations that a discussion provides.

from csharplang.

Thaina avatar Thaina commented on July 17, 2024

Interesting idea
But I don't like the way that we use as >= 0 and expect int? because the syntax should match float and any object that implement interface IEqualityOperators<int>

from csharplang.

IS4Code avatar IS4Code commented on July 17, 2024

@Thaina I don't like it either, but that is how patterns work in general ‒ x is >= 0 checks for int.

from csharplang.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.