Changed parsing code for the phonenumbers fields to incorporate different com...

Changed parsing code for the phonenumbers fields to incorporate different combinations of the phone fields in the forms.
Should fix a lot of the phone parsing bugs and make changes to parsing code much easier.
BUG=71893, 71897
TEST=unit-tested, please also check that any web-form that includes a phone number fields gets that fields parsed correctly

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75368

[dhollowa, 2011-02-16 00:25:37]

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/form...
File chrome/browser/autofill/form_structure_unittest.cc (right):

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/form...
chrome/browser/autofill/form_structure_unittest.cc:1124: // passed phone will be
parsed as CC-AC-PHONE.
Suggestion: ...phone will be parsed as <country code> - <area code> - <phone>.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_field.cc (right):

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:26: { PhoneField::REGEX_SEPARATOR, 0, 0
},
The second zero should be |FIELD_NULL| and throughout.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:172: memset(parsed_phone_fields_, 0,
sizeof(AutoFillField*) * FIELD_MAX);
How about |sizeof(parsed_phone_fields_)| instead?

It might be better to consider using a vector instead.  memset seems so 1973.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:264: memset(parsed_fields, 0,
sizeof(AutoFillField*) * FIELD_MAX);
sizeof(parsed_phone_fields_)

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:274: phone_field_grammars_[i].max_size <
nit: indentation looks wrong here.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_field.h (right):

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.h:34: enum PHONE_TYPE {
Types should be mixed case, not all-caps.  So: s/PHONE_TYPE/PhoneType/.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.h:60: enum REGEX_MATCH {
Types should be mixed case, not all-caps.  So: s/REGEX_MATCH/RegexMatch/ or
maybe "RegexType"?

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.h:96: enum {
Let's add a type to this enum, "PhonePart" say (?), and then use that this type
definition on line 112 to type the |field_id|.  And maybe change that from
|field_id| to |phone_part|.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.h:97: FIELD_COUNTRY_CODE,
I notice the |phone_field_grammars_| member is initialized with 0 for the
PhoneField::REGEX_SEPARATOR.  It would be preferable to have an explicit
FIELD_NULL = -1, at the start to avoid value overloading of 0 ==
FIELD_COUNTRY_CODE.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.h:112: size_t field_id;    // Index of the
field.
As mentioned above, size_t -> PhonePart, and field_id -> phone_part.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_field_unittest.cc (right):

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field_unittest.cc:174: // Phone in format
field-field-field could be either area code-prefix-
I had a bit of trouble parsing this comment.  I might suggest rewording as:

Phone in format <field> - <field> - <field> could be either <area code> -
<prefix> - <suffix>, or <country code> - <area code> - <phone>. The only
distinguishing feature is size: <prefix> is no bigger than 3 characters, and
<suffix> is no bigger than 4.

http://codereview.chromium.org/6480083/diff/5001/chrome/test/data/autofill/he...
File chrome/test/data/autofill/heuristics/input/form_phones_en.html (right):

http://codereview.chromium.org/6480083/diff/5001/chrome/test/data/autofill/he...
chrome/test/data/autofill/heuristics/input/form_phones_en.html:26: Phone: <img
src="https://securepics.ebaystatic.com/aw/pics/s.gif" width="1">
Let's get rid of the <img> elements.  They're not needed.

[Ilya Sherman, 2011-02-16 09:33:22]

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_resources.grd (right):

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_resources.grd:124: ^-$|\)$
nit: You have "^" only on the left, but "$" both on the left and on the right --
is that what you want?

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_field.cc (right):

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:26: { PhoneField::REGEX_SEPARATOR, 0, 0
},
On 2011/02/16 00:25:37, dhollowa wrote:
> The second zero should be |FIELD_NULL| and throughout.

I assume you mean the first zero?

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:27: // Phone: CCFIELD ACFIELD - PHONE -
SUFFIX (Ext: EXT)?
I'm trying to understand the comment structure you have here.  Two questions:

1) Why does the comment not specify "PHONE:3"?  Is that just a typo?
2) What distinguishes "(Ext: EXT)?" and "(- EXT)"?  None of the patterns seem to
directly include extension regexes.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:86: };
I'm finding this list pretty hard to grok.  Do you have an idea of how this will
extend to the international phone number case?  Could we generate these patterns
programmatically, rather than listing each by hand?

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:130:
DCHECK(parsed_phone_fields_[FIELD_PHONE]);  // Phone was incorrectly parsed.
nit: I think you mean "was _correctly_ parsed"?

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:288: return false;
Seems like we could fill parsed_fields[FIELD_PHONE] and then fail to parse a
suffix for the final pattern -- we should return 'false' in that case, but
wouldn't with this check.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_field.h (right):

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.h:49: // field4 means Country Code, Area
Code, Preffix, Suffix)
nit: "Prefix" (one 'f')

[dhollowa, 2011-02-16 16:40:03]

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_field.cc (right):

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:26: { PhoneField::REGEX_SEPARATOR, 0, 0
},
Yes.

On 2011/02/16 09:33:22, Ilya Sherman wrote:
> On 2011/02/16 00:25:37, dhollowa wrote:
> > The second zero should be |FIELD_NULL| and throughout.
> 
> I assume you mean the first zero?

[GeorgeY, 2011-02-16 20:53:34]

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/form...
File chrome/browser/autofill/form_structure_unittest.cc (right):

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/form...
chrome/browser/autofill/form_structure_unittest.cc:1124: // passed phone will be
parsed as CC-AC-PHONE.
On 2011/02/16 00:25:37, dhollowa wrote:
> Suggestion: ...phone will be parsed as <country code> - <area code> - <phone>.

Done.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_field.cc (right):

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:26: { PhoneField::REGEX_SEPARATOR, 0, 0
},
On 2011/02/16 00:25:37, dhollowa wrote:
> The second zero should be |FIELD_NULL| and throughout.

Changed to FIELD_NONE

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:26: { PhoneField::REGEX_SEPARATOR, 0, 0
},
On 2011/02/16 09:33:22, Ilya Sherman wrote:
> On 2011/02/16 00:25:37, dhollowa wrote:
> > The second zero should be |FIELD_NULL| and throughout.
> 
> I assume you mean the first zero?
Yes, fixed

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:27: // Phone: CCFIELD ACFIELD - PHONE -
SUFFIX (Ext: EXT)?
On 2011/02/16 09:33:22, Ilya Sherman wrote:
> I'm trying to understand the comment structure you have here.  Two questions:
> 
> 1) Why does the comment not specify "PHONE:3"?  Is that just a typo?
typo - fixed.
> 2) What distinguishes "(Ext: EXT)?" and "(- EXT)"?  None of the patterns seem
to
> directly include extension regexes.
(Ext: EXT)? means it is optional pattern parsed separately from the match here
(see the comment above the array). The second pattern was not correct - 
patterns updated.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:86: };
On 2011/02/16 09:33:22, Ilya Sherman wrote:
> I'm finding this list pretty hard to grok.  Do you have an idea of how this
will
> extend to the international phone number case?  Could we generate these
patterns
> programmatically, rather than listing each by hand?

Probably would be difficult to generate them programmatically. The
internationalization is build-in in autofill.grd (the names of the patterns are
changed).
If we find new structures of the phone we should add them there (I am not sure
only about R2L languages - the fields should be processed in correct R2L
sequence, but I am not sure if the phones are turned around L2R).

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:130:
DCHECK(parsed_phone_fields_[FIELD_PHONE]);  // Phone was incorrectly parsed.
On 2011/02/16 09:33:22, Ilya Sherman wrote:
> nit: I think you mean "was _correctly_ parsed"?

I written when DCHECK hits :). As we should encompass all the cases, we either
parse or we do not, we should not have half-parsed phones. That is why this
check is here - changed the comment.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:172: memset(parsed_phone_fields_, 0,
sizeof(AutoFillField*) * FIELD_MAX);
On 2011/02/16 00:25:37, dhollowa wrote:
> How about |sizeof(parsed_phone_fields_)| instead?
> 
> It might be better to consider using a vector instead.  memset seems so 1973.
:)
Done. The vector is not there because: 1. The size is always the same. 2. The
pointers are not owned by the PhoneField class, so I cannot put anything but the
pointers in the vector, thus requiring zero initialization anyway.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:264: memset(parsed_fields, 0,
sizeof(AutoFillField*) * FIELD_MAX);
On 2011/02/16 00:25:37, dhollowa wrote:
> sizeof(parsed_phone_fields_)

Done.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:274: phone_field_grammars_[i].max_size <
On 2011/02/16 00:25:37, dhollowa wrote:
> nit: indentation looks wrong here.

Indented additional 2 spaces.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:288: return false;
On 2011/02/16 09:33:22, Ilya Sherman wrote:
> Seems like we could fill parsed_fields[FIELD_PHONE] and then fail to parse a
> suffix for the final pattern -- we should return 'false' in that case, but
> wouldn't with this check.

Suffix is optional - it gets parsed iff (if and only if :)) it was not parsed
already *and* suffix pattern matches.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_field.h (right):

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.h:34: enum PHONE_TYPE {
On 2011/02/16 00:25:37, dhollowa wrote:
> Types should be mixed case, not all-caps.  So: s/PHONE_TYPE/PhoneType/.

Done.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.h:49: // field4 means Country Code, Area
Code, Preffix, Suffix)
On 2011/02/16 09:33:22, Ilya Sherman wrote:
> nit: "Prefix" (one 'f')

Done.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.h:60: enum REGEX_MATCH {
On 2011/02/16 00:25:37, dhollowa wrote:
> Types should be mixed case, not all-caps.  So: s/REGEX_MATCH/RegexMatch/ or
> maybe "RegexType"?

Done. (RegexType)

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.h:96: enum {
On 2011/02/16 00:25:37, dhollowa wrote:
> Let's add a type to this enum, "PhonePart" say (?), and then use that this
type
> definition on line 112 to type the |field_id|.  And maybe change that from
> |field_id| to |phone_part|.

Done.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.h:97: FIELD_COUNTRY_CODE,
On 2011/02/16 00:25:37, dhollowa wrote:
> I notice the |phone_field_grammars_| member is initialized with 0 for the
> PhoneField::REGEX_SEPARATOR.  It would be preferable to have an explicit
> FIELD_NULL = -1, at the start to avoid value overloading of 0 ==
> FIELD_COUNTRY_CODE.

Done.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.h:112: size_t field_id;    // Index of the
field.
On 2011/02/16 00:25:37, dhollowa wrote:
> As mentioned above, size_t -> PhonePart, and field_id -> phone_part.

Done.

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_field_unittest.cc (right):

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field_unittest.cc:174: // Phone in format
field-field-field could be either area code-prefix-
On 2011/02/16 00:25:37, dhollowa wrote:
> I had a bit of trouble parsing this comment.  I might suggest rewording as:
> 
> Phone in format <field> - <field> - <field> could be either <area code> -
> <prefix> - <suffix>, or <country code> - <area code> - <phone>. The only
> distinguishing feature is size: <prefix> is no bigger than 3 characters, and
> <suffix> is no bigger than 4.

Done.

http://codereview.chromium.org/6480083/diff/5001/chrome/test/data/autofill/he...
File chrome/test/data/autofill/heuristics/input/form_phones_en.html (right):

http://codereview.chromium.org/6480083/diff/5001/chrome/test/data/autofill/he...
chrome/test/data/autofill/heuristics/input/form_phones_en.html:26: Phone: <img
src="https://securepics.ebaystatic.com/aw/pics/s.gif" width="1">
On 2011/02/16 00:25:37, dhollowa wrote:
> Let's get rid of the <img> elements.  They're not needed.

Done.

[dhollowa, 2011-02-16 21:19:19]

LGTM.  Minus one correction to the regex.  Thanks!

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_resources.grd (right):

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_resources.grd:124: ^-$|\)$
Yes, this was a mistake in the old regex as well.  It should be ^-$|^\)$
On 2011/02/16 09:33:22, Ilya Sherman wrote:
> nit: You have "^" only on the left, but "$" both on the left and on the right
--
> is that what you want?

[Ilya Sherman, 2011-02-16 23:22:08]

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_field.cc (right):

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:172: memset(parsed_phone_fields_, 0,
sizeof(AutoFillField*) * FIELD_MAX);
On 2011/02/16 20:53:34, GeorgeY wrote:
> On 2011/02/16 00:25:37, dhollowa wrote:
> > How about |sizeof(parsed_phone_fields_)| instead?
> > 
> > It might be better to consider using a vector instead.  memset seems so
1973.
> :)
> Done. The vector is not there because: 1. The size is always the same. 2. The
> pointers are not owned by the PhoneField class, so I cannot put anything but
the
> pointers in the vector, thus requiring zero initialization anyway.

Please remind me: Who owns the pointers?  Do we clean up memory appropriately if
we try to parse a pattern and only match half of it?

http://codereview.chromium.org/6480083/diff/8001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:20: // separately unless they are
necessary part of the match.
nit: part -> parts

http://codereview.chromium.org/6480083/diff/8001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:21: PhoneField::Parser
PhoneField::phone_field_grammars_[] = {
nit: Perhaps: "The following notation is used to describe the patterns:"?

http://codereview.chromium.org/6480083/diff/8001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:28: { PhoneField::REGEX_PHONE,
PhoneField::FIELD_COUNTRY_CODE, 0 },
nit: patter -> pattern, matche d -> matched

http://codereview.chromium.org/6480083/diff/8001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:297: if (!ParseText(&q,
phone_field->GetSuffixRegex(),
Suppose the final pattern were "// Phone: <ac> - <phone>:3 - <suffix>:4 (Ext:
<ext>)?".  We could match "<phone>:3" and fail to match "<suffix>:4", which
would result in |parsed_fields[FIELD_PHONE]| being non-NULL, but we would not
actually have successfully parsed the pattern.

[GeorgeY, 2011-02-17 00:21:18]

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_resources.grd (right):

http://codereview.chromium.org/6480083/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_resources.grd:124: ^-$|\)$
On 2011/02/16 21:19:19, dhollowa wrote:
> Yes, this was a mistake in the old regex as well.  It should be ^-$|^\)$
> On 2011/02/16 09:33:22, Ilya Sherman wrote:
> > nit: You have "^" only on the left, but "$" both on the left and on the
right
> --
> > is that what you want?
> 

Done.

http://codereview.chromium.org/6480083/diff/8001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_field.cc (right):

http://codereview.chromium.org/6480083/diff/8001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:20: // parsed separately unless they are
necessary part of the match.
On 2011/02/16 23:22:08, Ilya Sherman wrote:
> nit: part -> parts

Done.

http://codereview.chromium.org/6480083/diff/8001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:21: // The following comment are
indicating the matched pattern:
On 2011/02/16 23:22:08, Ilya Sherman wrote:
> nit: Perhaps: "The following notation is used to describe the patterns:"?

Done.

http://codereview.chromium.org/6480083/diff/8001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:28: // (pattern <field>)? means patter is
optional and matche d separately.
On 2011/02/16 23:22:08, Ilya Sherman wrote:
> nit: patter -> pattern, matche d -> matched

Done.

http://codereview.chromium.org/6480083/diff/8001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:297: return false;
On 2011/02/16 23:22:08, Ilya Sherman wrote:
> Suppose the final pattern were "// Phone: <ac> - <phone>:3 - <suffix>:4 (Ext:
> <ext>)?".  We could match "<phone>:3" and fail to match "<suffix>:4", which
> would result in |parsed_fields[FIELD_PHONE]| being non-NULL, but we would not
> actually have successfully parsed the pattern.

Nope. We will not get here until we have a complete match - the check at #278
going to fail and then the check @ #290, so we proceed to the next pattern.

[Ilya Sherman, 2011-02-17 00:35:12]

http://codereview.chromium.org/6480083/diff/8001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_field.cc (right):

http://codereview.chromium.org/6480083/diff/8001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:297: return false;
On 2011/02/17 00:21:18, GeorgeY wrote:
> On 2011/02/16 23:22:08, Ilya Sherman wrote:
> > Suppose the final pattern were "// Phone: <ac> - <phone>:3 - <suffix>:4
(Ext:
> > <ext>)?".  We could match "<phone>:3" and fail to match "<suffix>:4", which
> > would result in |parsed_fields[FIELD_PHONE]| being non-NULL, but we would
not
> > actually have successfully parsed the pattern.
> 
> Nope. We will not get here until we have a complete match - the check at #278
> going to fail and then the check @ #290, so we proceed to the next pattern.

But there is no next pattern after the final pattern.

[GeorgeY, 2011-02-17 01:12:24]

http://codereview.chromium.org/6480083/diff/8001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_field.cc (right):

http://codereview.chromium.org/6480083/diff/8001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_field.cc:297: return false;
On 2011/02/17 00:35:12, Ilya Sherman wrote:
> On 2011/02/17 00:21:18, GeorgeY wrote:
> > On 2011/02/16 23:22:08, Ilya Sherman wrote:
> > > Suppose the final pattern were "// Phone: <ac> - <phone>:3 - <suffix>:4
> (Ext:
> > > <ext>)?".  We could match "<phone>:3" and fail to match "<suffix>:4",
which
> > > would result in |parsed_fields[FIELD_PHONE]| being non-NULL, but we would
> not
> > > actually have successfully parsed the pattern.
> > 
> > Nope. We will not get here until we have a complete match - the check at
#278
> > going to fail and then the check @ #290, so we proceed to the next pattern.
> 
> But there is no next pattern after the final pattern.

After the last pattern we bail out of the function @289