Page MenuHomePhabricator

Raise a type error if resulting value is incompatible with argument type
Open, In Progress, MediumPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

What happens?:
The function executes without error but different implementations pass or fail differently. In particular, a Composition testing for the presence of not(False) fails to identify the (non-Boolean) list as not being False https://www.wikifunctions.org/view/en/Z13457

What should have happened instead?:
It should not be possible to add a list as an element in a Boolean list, so it should not be possible to add a function that returns a list.

If it is possible to add such a function, it should not evaluate without an error.

In any event, a list should not be considered to be equivalent to Boolean False. (In this Test case, the list is specified to contain the string “not a Boolean”.)

Software version (skip for WMF-hosted wikis like Wikipedia):

Other information (browser name/version, screenshots, etc.):

Details

TitleReferenceAuthorSource BranchDest Branch
Eagerly evaluate arguments before validatingrepos/abstract-wiki/wikifunctions/function-orchestrator!153apineapine-evaluate-before-validatemain
Customize query in GitLab

Event Timeline

The function call

prepend element to list("not a Boolean", [Object ] )

is fine. The second parameter is an empty UNtyped list, so evaluating that should return in the untyped list with a single element, the String "not a Boolean":

[Object "not a Boolean"]

are all false( [Object "not a Boolean"] )

should raise a type validation error, because the argument type (typed list of Booleans) is incompatible with the given value.

The original title of this task is "A Typed list of Booleans can contain a Typed list", but that is not what is happening here: there is no typed list of booleans here that contains a typed list.

I am changing the title of the task instead to "Raise a type error if resulting value is incompatible with argument type"

DVrandecic renamed this task from A Typed list of Booleans can contain a Typed list to Raise a type error if resulting value is incompatible with argument type.Feb 29 2024, 9:04 PM
DVrandecic removed DVrandecic as the assignee of this task.
DVrandecic subscribed.

Thanks, Denny. I don’t disagree with what you say, but I’m not sure about your new title.

The question in my mind is about the timing of the “type error” exception. As I said, “It should not be possible to add a list as an element in a Boolean list, so it should not be possible to add a function that returns a list.” It doesn’t matter whether the list is typed or untyped; what matters is that it cannot be a Boolean, come what may.

So, *ideally* the user should be politely advised not to use such a function. From your amended title, it appears that you intend to allow the embedded function call to be specified but reject after its evaluation. (Unless by “resulting value” you mean Z8K2/return type.) I think we should reserve that approach for cases where the embedded function call may or may not return a value of an appropriate type.

Even then, a gentle warning seems appropriate. I’m not suggesting we should type-check the whole composition, just provide feedback when a function call is used as an argument and its return type does not match the argument’s type.

My third observation (“a list should not be considered to be equivalent to Boolean False”) should not be overlooked entirely, but it may be my logic is at fault here. Or it may be the contains function. Or something else (in which case, I guess that should be a different bug).

I’m sorry if my original title led to any confusion. It happens to be true but, as you correctly observe, in this particular case the list is untyped (although it proclaims itself to be a “Typed list” on the screen and the outcome is the same if it is changed to to be a list of strings).

My two cents:

I would be careful about us doing too much type-checking in the frontend. Type-checking in Wikifunctions is not a decidable problem--we can't always answer the question about whether an object is of a given type without running a function.

If we accept a Boolean as input, we can statically check the type of an input object in some cases. We know it's a Boolean if its type is Z40 (or a Z4 whose Z4K1 field is Z40, etc.). As you say, we could conceivably also do so if the input is a function call whose function's return type is Z40. But if that function call's Z7K1/Function is given as another function call (one that returns Z8), we can't say--we won't know, without running that function, whether the resulting Z8's return type is Z40 or not.

As an aside: we do have a proposal to address this (making Z8 itself a generic type, parameterized by its argument and return types), but that proposal is very far from reality. That would move us partway toward a more complete ability to do static type-checking.

But even in that case, we can construct situations where it's impossible to figure out what's going on without running a function call: if I were a wicked person with a lot of free time, I could use Z808/Abstract to make it utterly impossible to know anything about the input 🤕 . That is why we tend to wait until a function call is run to check things: if we're going to delegate to the orchestrator anyway, we may as well wait until runtime to do these kinds of checks.

That said, it would be possible to statically check your case, given that the return types of the functions are known. From my point of view, the question is how valuable would be to implement that logic, given all the ways it can fail. If we did implement that logic and surface warnings, we'd sometimes be able to provide useful information, like, "You provided an object of type ZXXXX when Z40 was expected." But in many, if not most, cases, the warning would be more noncommittal, e.g., "Could not determine whether object of type <generic type> is of type Z40."

(To be clear: the changed scope for this task is absolutely something we can and should fix in the backend; my comment above concerns the wider question of frontend type-checking)

I haven't replicated this yet, but I think I see the problem.

When we retrieve an argument from the scope, we validate it before we "eagerly evaluate" it (i.e., before we resolve any internal Z7s and the like). Our validation is necessarily permissive about Z7s, Z9s, and Z18s, so it lets this through. I can move the validation step until after the eager evaluation step (which is where it should be anyway). This is, unfortunately, the volatile part of the orchestrator code, so I may be dealing with some explosions.

Okay, I have now replicated the bug. A partial fix is out for review. The rest of the fix involves a big-ish change; the team will discuss that change soon.

cmassaro changed the task status from Open to In Progress.Apr 4 2024, 3:05 PM
cmassaro moved this task from 24Q4 (Apr–Jun) to In Progress on the Abstract Wikipedia team board.

My two cents:

I would be careful about us doing too much type-checking in the frontend.

Thank you for your thoughts, Cory. I agree…broadly. In my “more ideal” world, if I elect to choose a function call to supply a value, I should only see functions with compatible return types in the search. Or perhaps I should see functions with incompatible return types “greyed out”. Or perhaps those with the required type could be flagged with a big green tick ✅ (⚠️ and 🛑 being deployed, as appropriate, for the possibly compatible and definitely incompatible types).

There is a limit, of course, but I think it is easier to nudge the innocent user in the right direction rather than let them construct some form of nonsense that might not be easy to explain with error messages from the Orchestrator.

… if that function call's Z7K1/Function is given as another function call (one that returns Z8), we can't say--we won't know, without running that function, whether the resulting Z8's return type is Z40 or not.

Here, I’m not sure I agree. If the outer function has the correct return type, we’re good ✅ If the inner function may or may not evaluate to that return type, we’re also good ⚠️ because if the inner function evaluates to an incompatible type, the outer function won’t accept it. This is why my original response to Denny queries the timing. It really is easier (to my mind) to check statically during the user interaction. I would have thought it would also be easier during Orchestration. What’s the point of trying to orchestrate an evaluation that cannot deliver a valid result? (I’m not suggesting that generally happens, of course, although this bug suggests it sometimes might. You say it is “necessarily permissive” about Z7s, and I say there are limits… Lists are never Boolean, so a function that must return a list (of elements with some indeterminate type) will certainly not be returning a Boolean (or a String, or a Natural number…), so we need only flag that error and move on (or give up).)

It may be that the outer function is type-indeterminate through indirection, of course. This is regrettable and perhaps ultimately unavoidable. Nevertheless, I think we should encourage the use of a thin Type-wrapper function as the general rule in such cases. But that’s just my opinion 😏

Yeah, the unfortunate truth is that it's very easy to construct ZObjects whose type can't be known until runtime. We can't, in general, know that an inner function evaluates to an incompatible type until we run it. For user-defined types and generic types, this will be true unless and until we decide to banish arbitrary function calls from the type calculus.

It is true that we can detect some cases. A List is one of those ... sort of. There are infinitely many ways to create a ZObject that resolves to a List (or a Boolean, for that matter). We can certainly tell that, e.g., {Z1K1: 'Z7', Z7K1: 'Z881', Z88K1: 'Z6'} isn't a Boolean. We could write some frontend logic for that. It would be a special case, and it would mean that some inputs would be treated differently from other inputs that are semantically equivalent. The question, ultimately, is how many of these special-case type-checking heuristics we want in the frontend, and how comfortable we are with different experiences for situations that, logically, shouldn't be different. I don't have the answer to that, so I'll wait for others weigh in.

…I'll wait for others weigh in.

Sure. My feeling is that it is very much the normal case, and the case we should strongly encourage, that the return type of a function, or the Type of a referenced object, should exactly correspond to the Type required by its immediate context, just as an entered value should.

Specifically:

…I’m not suggesting we should type-check the whole composition, just provide feedback when a function call is used as an argument and its return type does not match the argument’s type.

(or, I suppose I should add, it does not match the Type of the value in the typed object… “immediate context” with the emphasis on immediate.)

Thanks!

…For user-defined types and generic types, this will be true unless and until we decide to banish arbitrary function calls from the type calculus.

💯By all means, banish arbitrary function calls!

I guess we have to compromise in the case of arbitrary types, but most proposed types are not arbitrary. https://www.wikifunctions.org/wiki/Wikifunctions:Type_proposals.

Someone may care to comment on https://www.wikifunctions.org/wiki/Wikifunctions:Type_proposals/configuration_of_functions_for_given_types from a technical perspective. I suppose it is formally arbitrary, so it is relevant in this context. I shall add to my comments there in due course. In the current context, I would suppose that a user would simply be warned ⚠️ that it is not type-safe (as if the return type were Z1).

Hello! For the moment, it looks like we will leave this task alone. The issue here is the backend validation workflow, which we've intentionally deactivated in certain circumstances. Re-enabling it isn't a high priority right now.

I don't have any answers about frontend type detection; I will leave that for @DVrandecic to think about.

Change #1020837 had a related patch set uploaded (by Jforrester; author: Jforrester):

[operations/deployment-charts@master] wikifunctions: Upgrade orchestrator 2024-04-04-132719 to 2024-04-17-125039

https://gerrit.wikimedia.org/r/1020837

Change #1020837 merged by jenkins-bot:

[operations/deployment-charts@master] wikifunctions: Upgrade orchestrator 2024-04-04-132719 to 2024-04-17-125039

https://gerrit.wikimedia.org/r/1020837

Hello! For the moment, it looks like we will leave this task alone. The issue here is the backend validation workflow, which we've intentionally deactivated in certain circumstances. Re-enabling it isn't a high priority right now.

I don't have any answers about frontend type detection; I will leave that for @DVrandecic to think about.

Thank you for your efforts. Should I raise a Feature Request for preventing inappropriate functions being selected in the front-end or is this expected to be addressed as custom types are rolled out?