New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Confusing error message for \1+ in regex #2110

Closed
jmaslak opened this Issue Jul 22, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@jmaslak
Contributor

jmaslak commented Jul 22, 2018

The Problem

A confusing error message is given when a particular invalid Perl 6 regex (that is something you would use in Perl 5) is used.

Example:

[0] red:jmaslak$ perl6 -e '"a" ~~ /(a)\1+$/'
===SORRY!===
Unrecognized backslash sequence (did you mean $0?)
at -e:1
------> "a" ~~ /(a)\1⏏+$/
Quantifier quantifies nothing
at -e:1
------> "a" ~~ /(a)\1+⏏$/
    expecting any of:
        infix stopper
        term

What is confusing is that there are two errors, when the problem isn't so much the +, but rather the \1. I should have seen that this was two errors being returned, but I focused on the one near the end and started wondering "What the heck is an infix stopper?!" The first error is awesome because it says exactly what you need to do to fix it, it's the second error that is confusing.

Expected Behavior

Tell the user \1 is wrong, nothing about quantifiers following.

Actual Behavior

See above.

Steps to Reproduce

See above.

Environment

  • Operating system: Ubuntu 18.04
  • Compiler version (perl6 -v):
This is Rakudo version 2018.06 built on MoarVM version 2018.06
implementing Perl 6.c.

@zoffixznet

This comment has been minimized.

Show comment
Hide comment
@zoffixznet

zoffixznet Jul 22, 2018

Contributor

Agreed. I'd look at the last one as well and think why is it telling it follows nothing when it does have stuff to follow.

"What the heck is an infix stopper?!"

Filed as #2115


Marking this as "good first issue". Tips for potential fixers for this:

The easiest fix is to just go into src/Perl6/Grammar.nqp, search for all occurences of "backslash sequence", and replace <.sorry("… with <.panic("…. There are several places where this message is thrown: in quote braid grammar as well as the regex one.

However, we do have a proper typed exception for this class of errors: X::Backslash::UnrecognizedSequence. So the more involved fix would be changing <.sorry to a <.typed_panic and giving it that exception to use as well as modifying that exception to take
suggestions as well.

Here's the diff of doing that for quote braid ("\1"). Needs to be done similar to this in two other places, (just search for "backslash sequence" to find them):

diff --git a/src/Perl6/Grammar.nqp b/src/Perl6/Grammar.nqp
index 81b4c1739..81e88480a 100644
--- a/src/Perl6/Grammar.nqp
+++ b/src/Perl6/Grammar.nqp
@@ -5075,11 +5075,6 @@ if $*COMPILING_CORE_SETTING {
 }
 
 grammar Perl6::QGrammar is HLL::Grammar does STD {
-
-    method throw_unrecog_backslash_seq ($sequence) {
-        self.typed_sorry('X::Backslash::UnrecognizedSequence', :$sequence);
-    }
-
     proto token escape {*}
     proto token backslash {*}
 
@@ -5101,8 +5096,18 @@ grammar Perl6::QGrammar is HLL::Grammar does STD {
         token backslash:sym<t> { <sym> }
         token backslash:sym<x> { :dba('hex character') <sym> [ <hexint> | '[' ~ ']' <hexints> | '{' <.obsbrace> ] }
         token backslash:sym<0> { <sym> }
-        token backslash:sym<1> { <[1..9]>\d* {} <.sorry("Unrecognized backslash sequence (did you mean \${$/ - 1}?)")> }
-        token backslash:sym<unrec> { {} (\w) { self.throw_unrecog_backslash_seq: $/[0].Str } }
+        token backslash:sym<1> {
+            <[1..9]>\d* {
+              self.typed_sorry: 'X::Backslash::UnrecognizedSequence',
+                :sequence(~$/), :suggestion('$' ~ ($/ - 1))
+            }
+        }
+        token backslash:sym<unrec> {
+          {} (\w) {
+            self.typed_sorry: 'X::Backslash::UnrecognizedSequence',
+              :sequence($/[0].Str)
+          }
+        }
         token backslash:sym<misc> { \W }
     }
 
diff --git a/src/core/Exception.pm6 b/src/core/Exception.pm6
index 2c64f12ae..3ae7d579f 100644
--- a/src/core/Exception.pm6
+++ b/src/core/Exception.pm6
@@ -1395,7 +1395,7 @@ my class X::Syntax::ParentAsHash does X::Syntax {
         "Syntax error while specifying a parent class:\n"
         ~ "Must specify a space between {$.parent.^name} and \{";
     }
-}  
+}
 
 my class X::Syntax::Malformed::Elsif does X::Syntax {
     has $.what = 'else if';
@@ -2142,7 +2142,11 @@ my class X::Cannot::Capture is Exception {
 
 my class X::Backslash::UnrecognizedSequence does X::Syntax {
     has $.sequence;
-    method message() { "Unrecognized backslash sequence: '\\$.sequence'" }
+    has $.suggestion;
+    method message() {
+        "Unrecognized backslash sequence: '\\$.sequence'"
+        ~ (nqp::defined($!suggestion) ?? ". Did you mean $!suggestion?" !! '')
+    }
 }
 
 my class X::Backslash::NonVariableDollar does X::Syntax {
Contributor

zoffixznet commented Jul 22, 2018

Agreed. I'd look at the last one as well and think why is it telling it follows nothing when it does have stuff to follow.

"What the heck is an infix stopper?!"

Filed as #2115


Marking this as "good first issue". Tips for potential fixers for this:

The easiest fix is to just go into src/Perl6/Grammar.nqp, search for all occurences of "backslash sequence", and replace <.sorry("… with <.panic("…. There are several places where this message is thrown: in quote braid grammar as well as the regex one.

However, we do have a proper typed exception for this class of errors: X::Backslash::UnrecognizedSequence. So the more involved fix would be changing <.sorry to a <.typed_panic and giving it that exception to use as well as modifying that exception to take
suggestions as well.

Here's the diff of doing that for quote braid ("\1"). Needs to be done similar to this in two other places, (just search for "backslash sequence" to find them):

diff --git a/src/Perl6/Grammar.nqp b/src/Perl6/Grammar.nqp
index 81b4c1739..81e88480a 100644
--- a/src/Perl6/Grammar.nqp
+++ b/src/Perl6/Grammar.nqp
@@ -5075,11 +5075,6 @@ if $*COMPILING_CORE_SETTING {
 }
 
 grammar Perl6::QGrammar is HLL::Grammar does STD {
-
-    method throw_unrecog_backslash_seq ($sequence) {
-        self.typed_sorry('X::Backslash::UnrecognizedSequence', :$sequence);
-    }
-
     proto token escape {*}
     proto token backslash {*}
 
@@ -5101,8 +5096,18 @@ grammar Perl6::QGrammar is HLL::Grammar does STD {
         token backslash:sym<t> { <sym> }
         token backslash:sym<x> { :dba('hex character') <sym> [ <hexint> | '[' ~ ']' <hexints> | '{' <.obsbrace> ] }
         token backslash:sym<0> { <sym> }
-        token backslash:sym<1> { <[1..9]>\d* {} <.sorry("Unrecognized backslash sequence (did you mean \${$/ - 1}?)")> }
-        token backslash:sym<unrec> { {} (\w) { self.throw_unrecog_backslash_seq: $/[0].Str } }
+        token backslash:sym<1> {
+            <[1..9]>\d* {
+              self.typed_sorry: 'X::Backslash::UnrecognizedSequence',
+                :sequence(~$/), :suggestion('$' ~ ($/ - 1))
+            }
+        }
+        token backslash:sym<unrec> {
+          {} (\w) {
+            self.typed_sorry: 'X::Backslash::UnrecognizedSequence',
+              :sequence($/[0].Str)
+          }
+        }
         token backslash:sym<misc> { \W }
     }
 
diff --git a/src/core/Exception.pm6 b/src/core/Exception.pm6
index 2c64f12ae..3ae7d579f 100644
--- a/src/core/Exception.pm6
+++ b/src/core/Exception.pm6
@@ -1395,7 +1395,7 @@ my class X::Syntax::ParentAsHash does X::Syntax {
         "Syntax error while specifying a parent class:\n"
         ~ "Must specify a space between {$.parent.^name} and \{";
     }
-}  
+}
 
 my class X::Syntax::Malformed::Elsif does X::Syntax {
     has $.what = 'else if';
@@ -2142,7 +2142,11 @@ my class X::Cannot::Capture is Exception {
 
 my class X::Backslash::UnrecognizedSequence does X::Syntax {
     has $.sequence;
-    method message() { "Unrecognized backslash sequence: '\\$.sequence'" }
+    has $.suggestion;
+    method message() {
+        "Unrecognized backslash sequence: '\\$.sequence'"
+        ~ (nqp::defined($!suggestion) ?? ". Did you mean $!suggestion?" !! '')
+    }
 }
 
 my class X::Backslash::NonVariableDollar does X::Syntax {
@jmaslak

This comment has been minimized.

Show comment
Hide comment
@jmaslak

jmaslak Jul 22, 2018

Contributor

Well, with guidance like that how could I not take a look at doing this? I'll see what I can get done. :)

Contributor

jmaslak commented Jul 22, 2018

Well, with guidance like that how could I not take a look at doing this? I'll see what I can get done. :)

@zoffixznet

This comment has been minimized.

Show comment
Hide comment
@zoffixznet

zoffixznet Jul 22, 2018

Contributor

Great. If you get stuck, you can ask here or join our dev IRC chat and ask there https://webchat.freenode.net/?channels=#perl6-dev

Contributor

zoffixznet commented Jul 22, 2018

Great. If you get stuck, you can ask here or join our dev IRC chat and ask there https://webchat.freenode.net/?channels=#perl6-dev

jmaslak added a commit to jmaslak/rakudo that referenced this issue Jul 22, 2018

Github Issue #2110 - improve err for obs positionals w/ quantifiers
Example: /\1+/ would previously give two messages, one about \1
being obs and "quantifier quantifies nothing".

Now the new behavior will just be to report the unrecognized backslash
sequence and offer a suggestion.

Thanks zoffixznet for determining what needed to be done and providing
some of the code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment