.Net Foundation Design Reviews, 21 Sep 2021

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: GitHub Quick Reviews

Description

Issues Discussed:
* dotnet/runtime 51700: Add new ObjectDisposedException constructor overload
https://github.com/dotnet/runtime/issues/51700
* dotnet/runtime 58607: Add reciprocal and SinCos methods to IFloatingPoint
https://github.com/dotnet/runtime/issues/58607
* dotnet/runtime 59061: Obsolete thumbtacked AssemblyName properties
https://github.com/dotnet/runtime/issues/59061
* dotnet/runtime 58047: Additional ArgumentNullException.ThrowIfNull overloads
https://github.com/dotnet/runtime/issues/58047
* dotnet/runtime 44738: Make it safer and easier to build an X500DistinguishedName
https://github.com/dotnet/runtime/issues/44738
* dotnet/runtime 1547: Add the ArchiveComment property for ZipArchive class
https://github.com/dotnet/runtime/issues/1547

Chapters:
0:00:00 Stream Start
0:03:35 Intro
0:04:18 Add new ObjectDisposedException constructor overload
0:16:36 Add reciprocal and SinCos methods to IFloatingPoint
0:19:10 Obsolete thumbtacked AssemblyName properties
0:23:19 Additional ArgumentNullException.ThrowIfNull overloads
0:27:51 Make it safer and easier to build an X500DistinguishedName
0:31:00 Add the ArchiveComment property for ZipArchive class
0:42:30 Wrap-up

A

A

A

B

Hello, everyone, it's tuesday, that's true and it's something like 10 am by which I mean 11 in an area around redmond washington. So let's talk api. um We because we're wrapping up a release. We gloriously don't have anything painted red, so we get to have a light day today and uh unfortunately, we're also light on attendance today, but hopefully that'll pick up in the next couple of minutes.

B

uh We promised an agenda ahead of time, so we're gonna not quite do things in order here, but this order instead, all right. This is me yeah. This is me all right: object, dispose exception. Constructor overload, 51700.

B

So we talked about this and we approved it.

B

As adding an object and type constructor to object disposed exception, steve gave a a comment after the meeting that maybe it should have been the static methods to just do the throw.

B

Emo replied in kind and then the reason that this is back up for review is the pr decided to go that route, and so now we have the pr implemented a suggestion, but what got approved was the constructors.

B

So the thing to review is changing it from constructors to static throws.

C

I mean it seems very reasonable because it doesn't do anything that you can't just do with the convenience helper anyway, and so it seems actually nicer to just shortcut it entirely right.

B

Yeah, I um I mean it'll mess up a little bit.

C

B

Flow analysis, unless the yeah.

C

That's the only thing that still pisses me off with the throw helpers.

B

But since this is just a basically- and you know, I have that- I expect it's, it's always the first thing and it's if disposed, throw you're not really going to have uninitialized variables after that, because you probably didn't declare them yet, like you probably didn't, declare anything until the dot dot here.

B

A

It's probably fine and it's for dotnet seven, so we have an entire release in which to beg or bug jared about. Let's make this actually work.

B

Yep, so the one uh outstanding question in both the pr and here is: should we say that these accept null values or not.

B

D

B

Yeah, I feel that you're always going to pass the literal, this or a literal type of and if you're doing anything else, you're weird and that um I'll be less I'll, be very prussian and say, and I don't care what you're doing emo did not laugh.

C

Sorry like what? What is the suggestion to have an analysis that says when you don't use this or type off, that's wrong? No.

B

So in the pr they made, whoever opened the pr made it support null. So if the type came in as null or the object came in as null, then it just went back to the default message and then they wanted to decorate it as null with the annotation saying no was permitted and I think that we should say no was not permitted and if that null refs well, why did you pass something other than this or a type of.

A

Well, the actual constructor supports null and if you pass in nothing it's null, so I don't see any reason why we wouldn't support these being nullable and then just treat them as null. Unless we may be explicitly wanted, one that doesn't take anything and then tell users, they have to use that.

C

I mean it seems more intuitive right.

B

Right, but the thing that takes null right now is a string like that's what message do you want to use? This is what object do you want to use as part of composing the message so they're they're different? That's also why it's an over or a method instead of an overload they're, semantically different.

A

Right, which is why I was saying: maybe we should also have one that takes nothing for users who want to throw this without passing anything, so you'd have throw that takes nothing. Throw that takes object, throw that takes tight.

C

I mean you, could I mean the general question, though, is like: when would you ever have that.

A

You don't want to expose the internal types that you're using.

C

Well but then you would basically still expose whatever the entry point was for the user right. Otherwise, it's not very helpful.

A

I mean maybe but there's all kinds of enterprises that don't want to leak their stuff.

B

Right and they can keep using the existing constructors.

C

Well, either that or yeah I don't know like. I have a hard time coming with an example where I don't want to throw something that has a helpful message but- and if you wanted to, I would just have an overload that takes nothing rather than making those parameters notable.

C

Because it seems weird to say throw now, it seems more intuitive to say just throw right at that point seems all logical.

B

Okay, I guess really it's: why would we, why would we make the static throw instead of just flip the default constructor from private to public.

D

To follow the same pattern in all cases.

B

Well, but we're introducing a new pattern. The current pattern is usain, throw new object, disposed exception and then usually what people pass is type of the type name that they're in dot full name. But it's a string, so people pass whatever they want.

C

Yeah, what do we have? We have just object name. Oh I see now. Okay, yeah.

C

I mean the one thing we could do is we could just provide the the string based thing that is nullable right.

C

That's the closest thing to the existing api surface and then you can just convert your code entirely to the throw pattern and it will always bind to something sensible, yeah.

A

In particular, asp.net in their components web view, in fact, a lot of asp.net does throw new object, disposed exception, object, name null.

A

They do an array builder uh circuits host.

A

They've got array builder duplicated a lot. That's why it looked like more than what I've called out so far, but.

C

Yeah, I'm curious whether that's you know has a good reason. Besides, I couldn't be bothered with producing a string here.

B

Because, yes, most of them are get type name or get type full name, and the original proposal was just add more overload. Add more constructors to just simplify the the pattern and so that we could stop having the full name versus name split and everybody would pass in a type or the this.

B

And then we would start getting consistent representation.

A

Yeah and then we'd avoid the issues in uh like iot, where they pass in a message as the object name thinking. That message is the first parameter.

B

C

That's also a good point: yeah.

B

But so I think we already have you know all of these, and now the ones that we're concerned about are when people want the.

B

I just want to say you know, type of the type that I'm in or if I want the full derived like. What's the runtime type of the object, instead of the declared type of the method then pass this yeah, and I think that those are the only two cases again. We can talk about taking this constructor that should only be called by the ee and making it public, because I don't know why it should only be called by the ee.

B

Obviously we support null so we can make that easier, but.

B

I assume that we did this because we had you know the us in the past had the opinion that you shouldn't pass null, but we made it work so.

C

I mean this is kind of how I read this comment right. It should only be called, but he seems more like well. If we don't have a message, then, because we don't have a message for this case right, but then the other one should have a message, but then somebody was defensively, programming and made knowledge just work right and that's, I think, how we got here.

C

I think the intent was. You always provide a message. That would be my guess at least.

C

Not message, sorry object, name, there's always going to be a message. It's just less helpful.

C

Object, name really yeah I mean I'm I'll, be okay with just having these two throw for now and then see what people say.

C

You can always add the third one. One.

B

D

I don't know if it is intentional, but the second throw is missing the stacked price fitting up.

B

Well, so I yeah- I copied this from a comment in the implementation because they in the implementation. They have this one deferring to this one, so they were getting uneven, uh stack heights, so yawn or somebody said, put stack trace hidden on the object.

C

Interesting, we do.

A

That for a lot of our throw helpers, because the throw helper itself is a helper and therefore it's not important to the stack trace, it's just an abstraction to help with inlining and stuff.

B

Right but they didn't suggest putting it on both. They just suggested balancing out the stack, because one public called the other.

A

I think we probably want it on both.

C

Yeah I mean I can't think of this in case. I want to see the helper on the top of the stick.

A

Yeah because, logically, the user should be able to do this, and then it be no difference to the debug ability of the program um to as if they had just done, throw directly.

C

And we mean we probably never able to fix the flow analysis problem, though, because it's not a c-sharp problem, really, I think it's an il correctness issue.

B

All right add reciprocal sign cost. I floating point: 58607 sounds like tanner.

A

Yep, this is basically um for generic math and finalizing the interfaces for dotnet seven. um There were a few math methods that we added to math and math f and dot net six, and so they should probably be on my floating point as well, specifically that sine cos and reciprocal estimate and reciprocal square root estimate.

D

What are the guidelines for returning to pulse on the framework.

C

We're okay with it so long, it's kind of invariant like meaning.

A

C

Will never add anything to it, but given the api is designed to return your sign in cosign, and can nothing ever do differently in the future? It seems fine.

D

Yeah they should.

C

D

C

D

Named what I was gonna say.

C

Yeah, that would be good yeah. I mean.

A

I would expect.

C

The first one to be signed in the second to be co-signed, but.

A

Yeah and that's exactly: how is it how it is in math and math.

B

Sine and cos, I assume, is what we call them. Yep.

B

Just making sure we didn't write them out with the ease.

A

You've got one too many triple ticks in there.

A

It's three! No, no, no oh, is that three dots not three triple ticks under the first brace.

B

Yeah so where I copied the diff I'll just delete those okay.

B

So we're making a breaking change to an interface.

A

No, it's not breaking, because it's a it's a preview interface right.

C

It's meant to be breaking indeed,.

A

Now now for net eight, these would have to be dims if we were adding them.

C

B

Day I'll have the time to finish writing down what all the ways they'll break us are.

B

All right, obsolete, thumbtacked assembly, name properties, nine, zero, six one. I think I saw that yawn's with us hi john.

E

Hello um yeah, so you know we run into this through some uh kind of related issue where we are looking at assembly name and what it does, and you know it occurs to us that these properties are the legacy, they kind of don't make sense and assemble the name, and they are not respected in dot net core, for example, some of them were never expected, for example, the version compatibility property. It was added as future proofing in.net framework and never did anything so.

F

B

Why are we obsoleting the get in the set instead of the property.

B

E

I don't know you know if it can be on the property itself and it just works, and you know it's certainly fine. So.

B

I think you can.

C

Yeah, I mean that's normally what we do I mean if you only want to obsolete one of them, I think it would make sense to put them on the accessor, but if we want to obsolete both directions, then I think putting them on the property itself makes more sense.

D

The new obsolete attribute, the one that contains the.

C

Diagnostic id: well, it's the same attribute right. It's just. We also set the diagnostic id, which we probably should.

B

C

Them diagnostic yeah, because I mean all the same reason right.

B

What's the name of that property.

C

B

C

Oh, I did not come up with that name.

B

Just reformatting things.

B

Can watch me struggle.

C

So one thing that is weird is that the project board that tracks runtime changes now has iot in the name. I don't understand that.

B

Okay, so I think that's what we want.

E

Looks good to me.

C

B

Thank you, young.

E

Yeah, thank you as well.

B

All right additional argument, no exception, throw if null overloads five eight zero. Four seven.

A

All right, so we approved throw if null that takes uh t for classes um that completely excludes pointers. uh Pointers are somewhat esoteric, so it's not necessarily required.

A

Or to also just support void, pointer here um for people who are doing interop and stuff and therefore have to deal with pointers regularly.

A

There was also the proposal for supporting nullable of tea for structs. I commented below, and others did as well, that.

A

Nullable of t for structs in particular, doesn't seem like it's a good fit. If it's nullable, then you shouldn't be throwing if it's null.

B

100 agree: yeah, I mean there's one minor error in that which is basically, if some other parameter says you know. If you've picked this mode, then you must now provide this option, but yeah, that's not just going to be the standard throw with the name of the parameter. You need a much more complicated message in that case right so so again, so there are cases where you would throw when a nullable is null. But it's not the simple case that this is represented.

B

I have no particular qualms for the void star version.

F

Typically, do you throw argument, no exception when you're past a void pointer.

A

I do because I don't think there's a better option. It's an argument. It's no no isn't allowed so, therefore thrown all.

F

E

You know there are like interesting traders. You know like there are actually a number of bucks that are caused by throwing for argument for point arguments being null when the pointer arguments points to buffer.

E

You know, historically, that number of metals that take buffers through, for uh you know, point to buffer being now, but that's not the right choice when the buffer size is zero and it causes like a lot of interesting corner case box.

A

And I think likewise, there's a lot of cases where you want to pass in null for the buffer. The first time that way, you can get the length. There's lots of windows apis that do that. But I think it's probably up to the user to understand when it's correct to check and throw for null versus, not if they're, making safe, safe wrappers over it.

B

B

A

Looks good to me.

B

No we're just sailing through these.

B

I picked this one for selfish reasons: okay, this one's funny, because we reviewed it, we approved it and then we decided that there was a layering problem with the proposal we had because system formats, asn 1 was not part of the shared runtime, but x509 was so.

B

We couldn't use it in x509 api, so we turned places that we needed an asn tag into an integer, um then for complicated reasons we had to put system formats as one publicly in the shared ref, um because otherwise it would break the building of net standard, dls forwarders, and so now it is there. So then kevin suggested that we go ahead and put it back and then now we've decided, we didn't need the full tag.

B

We just needed the tag number, which is what the cent was so basically we're changing an end to a nullable enum. I don't think anyone cares. Does anyone care.

F

That api hasn't shipped before obviously correct yeah. Then I don't care.

C

So why was that an end before.

B

So way back here, originally we had asn one nullable asn 1 tag, but then we ended up with a a reference assembly inversion problem and we actually couldn't expose this from the ref without breaking the build.

B

Then things changed during dot net five or six, and now we can and so where we moved it to an end, to fix the inversion which was.

C

So basic type folded, the thingy down is that what we did.

B

Yeah, so here we had made it an int which was just using the same value as the enum, and we were just going to pass it through with a cast and now we're saying it is an enum, so we're putting the enum back.

B

But since the enum has a semantic zero value, we're going back to having it be nullable like we did in the original proposal. Yep that makes sense.

B

0 is not a valid value, but because it's semantic we wanted to make a distinction between you wanted the default, and you asked for an invalid thing, all right approved for the third time.

F

So just this whole class hasn't ever shipped right. I just proved it: okay,.

B

All right add the archive comment: property for the zip archive class 1547 wow, that's old.

F

Do we have anybody that can speak to it.

B

It looks like add two properties.

C

F

C

They are in the standardized format.

C

Expose it before.

B

So apparently we're already reading them. We just didn't expose them.

C

B

So we read them out of the file format. We just didn't expose them, so this one makes sense.

B

So the one to me, the one on zip archive entry, makes total sense as get set because.

C

In there, I think it's good, then.

B

Right because you could be you know, building a new entry or you've opened the thing in update mode. What I don't understand, looking at what they've shown here for zip archive, is how this setter works.

B

Because what do you think that's why they asked the.

F

Question on that first, the first question: should we add a constructor.

C

I mean zip archive is mutable right because once you're building one, you basically get to add entries right. That's what the create entry thing does. So in that sense, having a setter there isn't.

B

Yeah, I guess there must just be another member that they're not showing, which is to save it.

C

Well, I mean it's disposed right.

C

Yeah, it's basically flush or dispose right, depending on your viewpoint,.

B

I guess yeah, it holds it all in memory until you dispose and then it flushes it down to the stream that was passed in the constructor.

C

um I guess it well yeah, I don't know. Actually I don't know how zip works. I think the I think the uh the the ways it works, I believe, is that the um you know the table with the entries is at the end. So I think if you write one file at a time, I think it can get those out and then, at the very end it writes the entries.

B

That's true um but an update. I wonder if an update mode you're allowed to remove things.

C

B

Doesn't look like it because there's no uh yeah, so I think it pushes all the stuff back out.

B

It pushes all the entries out or and then positions the the read write head between the last entry and the zip table: okay um yeah, so the setter will throw presumably they throw in valid operation exception and create entry when they're in read mode. The setter should also throw in read mode.

B

um You should also.

F

Throw if it's too long, I think, there's a max I'm sure.

B

This is just the um because they're mode based they'll have extra throws, and I don't think it's enough that we would make it that we would say it shouldn't, be a property.

F

Enough throwing you mean.

B

Yeah that, because, if you open it in read mode and then assign comment, it'll throw that the comment can't be assigned in read mode.

B

But if you open it and update or write, it should work. And if it has, you know a maximum length, then sure that's just argument. Validation.

B

All right, then they have here. Should it be, you know, string or string. um I love that use. The same number of questions uh depends on what's what's.

F

B

Yeah I mean my my feeling on that is. It depends on what the file format says. If it's a, if it's a record or if it's a piece of the data structure, that's always there and it might be empty. Then the empty string seems like the honest representation, and if it's a, if it's an optional record, then representing that it wasn't present versus the empty string seems semantic.

B

So I would say that that depends on the spec.

B

Which would be, you can certainly write in a comment.

F

Here about when you're, first creating one.

F

For zip archive at least well.

B

For both of them, I get for both of them. If it's a mandatory field right like if it's just you have to write down, there's a zero length string here, then starting it off a string. That empty seems the most honest answer.

B

Unless the thing that we actually start things off with is created with net, in which case we should. You know, admit that that's the answer.

B

But if it's optional and we're not doing it, then string question mark makes sense.

B

So I think the answer is we're the wrong people to answer that, because it's uh it's to me it's whatever the the data spec says.

F

Well, in the additional details, it says for the archive it's defined by the at the end of the central record, a two byte comment length and then the actual comment itself yeah. So.

B

It sounds like they're, not optional,.

F

Right because you always have to put the length in there.

B

F

So you're thinking it should just be system.string no question mark.

B

That is my feeling.

B

I could live with that yeah, it's just in the middle here.

C

Suppose you're saying there's no difference between now and empty.

F

Right so I could never set it to null. Then.

F

If we don't put the question mark.

C

Well, I mean we can't have either direction right. We could annotate it such that it's you know never returns now, but can be set to now. If you wanted to.

B

Yeah I mean the setter is perfectly it's perfectly fine for it to normalize nolan to empty.

B

But I think that the reader should materialize the zero length comment as the string dot empty, instead of as null.

C

Yeah, which is also why it's probably easier to just mark it as no no.

F

I didn't have the center it allowed now.

C

Yeah, I can never remember like it's either this way or the other way around.

B

All right yeah, what is that attribute? Allow null.

C

I believe so yeah.

B

And it goes on the property.

B

Stupid diff spacing.

B

I'm still wrong.

B

B

All right does that look right.

F

I think technically that goes on the setter, but.

B

F

I think it's easy enough understood.

B

um Not according to this, it goes on properties or parameters or fields, not on methods. Okay, and that was wrong.

B

Yeah, I only learned that you know I assumed it was going to go on the set as well, but apparently we designed it in a sensible way that odd all right, unless any of you have quickly marked things as ready for review.

B

I think that means, even though I was scolding emo for delaying my lunch time unnecessarily. We're actually going to still end early.

C

That's what I predicted: that's why I cut it down to one hour.

C

All right, then this is it.

B

I think I've been neglecting youtube chat so seeing if there's anything, we missed.

B

So someone asked for the dispose. What should we do if the value will be null? Well, since the thing that you're passing in is describing what object was already disposed? It shouldn't be null because if it was null, that would have been a different exception.

B

And that's really the only question so all right! Oh, I have a thing for this, so we have nothing left on the agenda and I have a gavel. So I declare this meeting adjourned. See you next time.

B

Yes- and I have a gavel.