[pigeon] Move test support logic out of the default Dart output.

[Hixie]

This will allow us to move the mock logic out of the core framework.

I also got distracted and changed some of the output to be cleaner while I was at it.

[Hixie]

flutter-team-archive/plugins#3281 applies these changes to video_player.

[gaaclarke]

I'm going to pick this up tomorrow, I got to it late and it's taking longer than I expected. Generally looking good I want to run it a few times while I think about it. I think the encode and decode methods was a good compromise while I'm looking at it.

[gaaclarke]

I find these variable names confusing. nullTag described it as a part of the language, nullable doesn't describe it as a character that has semantic meaning. I checked the Dart documentation to see what they call the ! operator and unfortunately I couldn't find a name. In other languages it's called unwrapping.

How about nullableTag and unwrapOperator or unwrapper?

[Hixie]

oh i didn't realise "null tag" was the official name. Happy to change these to whatever you want.

[Hixie]

done, ?=nulltag and !=unwrapOperator

[gaaclarke]

Why did you change this? Because dynamic doesn't denote null-safety? Why was there a dynamic and an Object before NNBD? Are some things not Objects?

[Hixie]

dynamic hides bugs, because it disables all type checking. By using Object? we can see where we're casting and we can make sure the methods (etc) we're using on those objects don't have typos.

[Hixie]

(To answer your questions, dynamic is essentially the same as Object?, except that the language never checks what you call on objects that are marked dynamic, it's like you're in an untyped language. Everything is an Object?.)

[gaaclarke]

Okay, finished up the review. The only big issues I noticed is that there is a null check that has been removed from the decode method. That makes me nervous, do you think that is correct and that it is adequately tested already? Also there is an added case where channels can fail silently and bypass the provided API.

[gaaclarke]

You need to use snake case, not kebob case here.

[Hixie]

Fixed.

[gaaclarke]

You do the same path juggling here twice, it could be more clear to pull some of that into a helper function.

[Hixie]

If you mean the code two lines above this, it's subtly different in ways that makes it hard to usefully factor out. :-(
(I spent some time trying to find a way to factor it out cleanly and it ended up just being simpler to do it this way.)
That said if you have any suggestions on how to factor it out let me know.

[gaaclarke]

Couldn't you just do this?

Stringposixify(String path) {
final path.Context context = path.Context(style: path.Style.posix);
return context.fromUri(path.toUri(path.absolute(path));
}

posixify(options.dartOut);
posixify(path.dirname(options.dartTestOut);

[Hixie]

The theory being that path.dirname(path.absolute(x)) == path.absolute(path.dirname(x))? Seems reasonable, let me give it a try.

[gaaclarke]

_fromMap used to accept null input, now decode doesn't. I see there appears to be one place where decode is called without a null check to the argument. Are you sure this is safe to remove?

[Hixie]

I must have misread the code; it looked like it used to crash when the message was null, but I see in the output for video_player that there's a null check.

I'm happy to make it accept null again.

(Is there ever a valid reason to send null?)

[Hixie]

Oh I remember why I made it not accept null -- it's not so much that the input is non-nullable, so much as that the output is non-nullable. With a null input I don't know what to return. I could return null but then I need to null check around every call site anyway, and it seemed cleaner to have the null check before the call than after the call.

[gaaclarke]

Yea, that makes sense. I just checked the output with nullsafety and it looks like this:

staticSearchReply_fromMap(Map<dynamic, dynamic> pigeonMap) {
finalSearchReply result =SearchReply();
result.result = pigeonMap['result'];
result.error = pigeonMap['error'];
return result;
}

Looks like I already got rid of the null check, this generated code just hasn't been updated in a while.

[gaaclarke]

We shouldn't bypass calling into the api. Either we support null here and pass it through to the api, or we don't and we assert. Silently bypassing the API is just going to create hard to debug situations.

[Hixie]

But then FlutterSearchApi.search has to take a nullable argument. Do we want that?

[Hixie]

I think this is the main open question here. (bold so I can find this in the github page)

[Hixie]

Ok sounds like from the previous comment that you're good leaving this as-is.

[gaaclarke]

Are these casts incurring any runtime cost?

[Hixie]

My understanding is that casts are cheaper than dynamic dispatch on dynamic these days, but I could be wrong.

[Hixie]

(To put it another way: there were casts before, this just made them visible.)

[Hixie]

(Specifically, before, we were assigning a dynamic (the return value of await channel.send(encoded)) to a variable of type Map<dynamic, dynamic>, which implies a cast. Now we are assigning an Object to a variable of type Map<Object, Object> with an explicit cast. I wouldn't be surprised if the assembly was actually byte for byte identical, though I haven't checked.)

[gaaclarke]

Here's the call whose behavior would change as a result of removing the null check in decode.

[Hixie]

Ah, yes. In null-safe mode this has a ! in the new generated code. I can check it for a result key and throw if it's missing, if you like. Or do something else. Your call.

[gaaclarke]

I think this is good. I got a confused by the skip in checking in generated code. Thanks ian.

[Hixie]

Comments applied, tests pass! I'ma land it!