Apply() can generate a Moodle Error

[Mat-Ge]

Clumsy use of apply() can generate a Moodle error. The problem only seems to occur in the question variables when the first argument is an array. For example, write "apply(a[1],[3]);" in the question variables. Of course, such a line without context is nonsense, but it should generate at most a STACK error and certainly not a Moodle error. I checked it with both of my systems (one with Moodle 4.4.3, the other with 4.3, both with STACK 4.7).

[Mat-Ge]

Different context, similar problem: If the following is inserted into the QV, a Moodle error results: a:b+1; c:a-a(d+1); Sure, I forgot the multiplication sign after the second "a", but this should at most produce a STACK error and not a Moodle error.

[sangwinc]

Sorry about this. To be honest, the error trapping is one of the most difficult parts of the whole project! But, we should probably not throw a Moodle error for sure here, since that leaves a question author at a dead end from which they can't recover. The problem is that the parser ends up getting back the expression "(b+1)-(b+1)(d+1)" from Maxima, and when we try to parse that we get the error (not from what you typed, but from what Maxima creates). Since this is an entirely internal problem, we throw an exception there, rather than an error to the user.

This is the line throwing the error https://github.com/maths/moodle-qtype_stack/blob/master/stack/cas/cassession2.class.php#L413

One option is to change that exception to throw an error attached the statement. Another option to to allow this kind of expression. Maxima does allow input like "(b+1)(d+1)".

@aharjula, do you have a view?

[aharjula]

@sangwinc well we have that runtime check for evil things to call inside Maxima happening just before the call. It could, in theory, check if the thing being called is not an identifier or string and error out there. Note that different types of "apply"-like constructs can generate different checks and different "applys" work with different types of "names" for the function and we do have two different types of tests constructed in 996.

That will have a cost, as the check will have to happen for everything. But in this case, we really should error out instead of returning something nonsensical we cannot parse.

Basically, to fix this, both %_E and %_C in Maxima need to error out if the thing they are checking is not an identifier or a string.

[sangwinc]

I think we need to do the check within Maxima itself, after we have evaluated expressions. The problem occurs within Maxima I think, and we need to check at the point we send values back from Maxima to PHP. E.g. in _CS2v. Is that the right place to check? The reason is that we can't parse the expression into an AST to apply filters.

[aharjula]

Well _CS2v would cut down the number of things that would need to be checked (compared to %_C and %_E), but it would also mean that you would need to check the whole expression instead of selected interesting bits.

However, there is a problem because the logic of _CS2v was never expected to generate errors, and thus, the session handling does not collect errors from it. Thus triggering error from within it would require catching that error...

[sangwinc]

Yes, @aharjula I'm just playing with all that....!

[sangwinc]

OK, so it turns out that some questions rely on sophisticated Maxima making use of application of compound functions. I'm sure that restricting something like this (i.e. legal in Maxima, but not in STACK) will break things, and so I'm going to propose an option which throws an error by default (which STACK catches, avoiding a Moodle exception) but which also allows an advanced question author the option to use this facility. I really don't want to needlessly break a whole bunch of advanced stuff!

[sangwinc]

@aharjula could you review and approve this proposed fix please? I'd very much value your thoughts on this. Thanks, Chris

[aharjula]

@sangwinc, maybe for the next parser we can allow this without having to do that rewrite to apply for output. It looks funny if outputted through {#...#}. However, I think it is unlikely that anyone is outputting those kinds of things to other tools through that and thus, it is probably not a massive regression.

Liking that I can surround just the things I want to have this, with the setting and get warnings for the other bits that I di into intend:

fun:(x+y);
fUn:(x+z);
OPT_APPLY_COMPOUND:true;
t:fun(x);
OPT_APPLY_COMPOUND:false;
T:fUn(x);
OPT_APPLY_COMPOUND:true;
X:apply(a[1],[x]);
OPT_APPLY_COMPOUND:false;

In any case, it did not seem to immediately break anything, but it will probably slow down CAS actions a bit as it is now executed for absolutely every single outputted function call and statement.

Some mistargeting of the errors, though. If I place {@fun(x)@} into question-text after those question-variables (with the first OPT_APPLY_COMPOUND:false; as true), I get the error within the teacher-answer of ans1. Same with trying to render inline castext inside question-text with the same content.

[sangwinc]

Thanks @aharjula. Are you able to help fix the error targeting? I'm not sure I fully understand the order in which things are executed/evaluated.

Chris

[aharjula]

Finally, got the time to look at this... A bit too big a change:

1. The editor form validation does not instantiate the CASText, so the runtime error does not trigger there.
2. The errors at runtime are correctly targetted, i.e., you will know that they came from the question text.
3. Instantiation during form validation might be too slow and expensive, so maybe not doing that would also need to be instantiated with suitable context, which is fine for question text and general feedback, etc., but not for PRT feedback.

As to why exactly we get errors in the first teachers' answers in the inputs, I actually misdiagnosed it. I had TA:t and in question variables fun:(x+y);OPT_APPLY_COMPOUND:true;t:fun(x);OPT_APPLY_COMPOUND:false;. Basically, t was fine in question variables, but after them, we also had OPT_APPLY_COMPOUND:false, so when reused in that context in the input, it caused an issue as it should. I just mixed it with triggering issues from CASText, which I thought I were triggering with setting that OPT_APPLY_COMPOUND:false... And those did not trigger from the CASText but from where they should have triggered i.e. from the input.

[sangwinc]

Thanks @aharjula. I've slept on this and propose we make this the first fix after we release 4.8.0. That gives us the whole development cycle to test and confirm we're happy. My proposed change is major in that it affects every interaction with the CAS. And we have questions which will break, and I'd like time to fix them. The actual problem is confined to a very specific edge case which is not really that common. We'll fix it, but the other risks are quite high....