parserAssignment chokes when previous answer is undefined on the domain

[drdrew42]

See #607 for details (incl. a MWE)

Submitting an answer whose domain is incompatible with the limits for the available variables causes an appropriate error message: The domain of your function doesn't match that of the correct answer

However, when followed up with an answer whose domain is compatible, you will get an error: The evaluated answer is not an answer hash : ||. (This happens only based on domain, and is independent of whether or not the submitted answer is correct.)

The failure to complete the processing of the answer hash is due to the cmp_postfilter which compares the current and previous answers.
Note: So long as the submitted answer generates a feedback message, the comparison between the current and previous submissions is skipped.
Specifically, the typeMatch method call (which ensures that the current and previous responses are comparable objects before actually comparing them) dies because the previous submission fails to generate valid points.

The solution is to simply remove typeMatch from parser::Assignment::Formula. It's not doing anything more than what (the superclass) Value::Formula is already doing. And Value::Formula::typeMatch is structured to handle situations where the generation of valid points fails. (Interesting to note: this looks like a bug that was fixed not long after parserAssignment was introduced -- but it was fixed for Value::Formula and the changes never made their way into parserAssignment... see 2e1ed6b)

TL;DR -- parser::Assignment::Formula doesn't need to override typeMatch

postfilter: 🍻 to @dlglin for finding a teenaged bug!

[drgrice1]

This looks good. Thanks for finding an old bug @dlglin. Thanks for fixing it @drdrew42.

[dpvc]

It's not doing anything more than what (the superclass)

That's actually not true. The super class (which is Value::Formula) typeMatch() function is

pg/lib/Value/AnswerChecker.pm

Lines 1653 to 1661 in 37637e2

	sub typeMatch {
	my $self = shift; my $other = shift; my $ans = shift;
	return 1 if $self->type eq $other->type;
	my $typeMatch = $self->getTypicalValue($self);
	$other = $self->getTypicalValue($other,1) if Value::isFormula($other);
	return 1 unless defined($other); # can't really tell, so don't report type mismatch
	return 1 if $typeMatch->classMatch('String') && Value::isFormula($ans->{typeMatch}); # avoid infinite loop
	$typeMatch->typeMatch($other,$ans);
	}

and it returns 1 immediately if $self->type equals $other->type. For a normal Formula, the type will be the type of the result of the formula (e.g., Number or Vector, etc.), and only goes on to try to analyze the return type if the named types don't match. For an assignment, however, the type is always Assignment. So using the super-class typeMatch() would always return 1 for any two assignments.

The current typeMatch() for assignments returns 0 if the types are not equal (i.e., if they aren't both assignments), and if they are both assignments, then it goes on to do additional type checking of the type of object that is being assigned to the variable. That additional checking would not be performed if you just use the Value::Formula->typeMatch(), and that checking is important for feedback messages. For example, in Vector context, if the correct answer is an assignment v = <1,2,3> and the student enters v = 1, the current typeMatch() would fail, and the message

Your answer isn't a variable equal to a vector
(it looks like a variable equal to a real number)

is issued. With the changes in this PR, however, the answer will be marked incorrect silently (no helpful message).

There are also other differences. The current typeMatch() forces other to be converted to a Formula object, but that should be able to be dropped, because it is already known to be an Assignment object, which is a Formula. But because the Assignment is actually a list-valued formula (the first item being the variable name and the second its value), the check for its assignment type is done on the second element from the list. That is not the case in Value::Formula->typeMatch(), which operates on the formula's value as a whole.

So this PR changes the functionality of the Assignment object from its current form in a way that I would not support.

It's true that the Assignment typeMatch() should be updated to use getTypicalValue() (which postdates this code),
so a possible version for the Assignment object might be

sub typeMatch {
  my $self = shift; my $other = shift; my $ans = shift;
  return 0 unless $self->type eq $other->type;
  my $typeMatch = $self->getTypicalValue($self)->{data}[1];
  $other = $self->getTypicalValue($other,1)->{data}[1];
  return 1 unless defined($other); # can't really tell, so don't report type mismatch
  return 1 if $typeMatch->classMatch('String') && Value::isFormula($ans->{typeMatch});  # avoid infinite loop
  $typeMatch->typeMatch($other,$ans);
}

though I haven't actually tried it out.

[dpvc]

See my comments above for suggested changes.

[drdrew42]

Following @dpvc's suggestion, the parserAssignment override of typeMatch has been restored, with the recommended changes to include getTypicalValue in place of what was there before.

Instead of using the line that is commented with 'avoid infinite loops', we instead check for Strings during object creation and throw an error.

[drgrice1]

There seems to be a problem created by this pull request. I have problems that use parserAssignment.pl and PGML's parsing to display equations. For example, one problem has [::[$e]/[$a] x + 1/[$b] = 1/[$c] x - 1/[$d]::]. This now gives the error

Errors parsing PGML:
Error parsing mathematics: No perl form for ' = '; see position 13 of formula

If I revert this pull request, the error does not occur.

[drgrice1]

Here is a minimal example for which this issue occurs:

DOCUMENT();

loadMacros(
	"PGstandard.pl",
	"MathObjects.pl",
	"PGML.pl",
	"parserAssignment.pl",
	"PGcourse.pl"
);

TEXT(beginproblem());

parser::Assignment->Allow;

BEGIN_PGML
>>[::2/5 x + 1/7 = 1/3 x - 1/11::]<<
END_PGML

ENDDOCUMENT();