ruby-core

Issue #19138 has been updated by Eregon (Benoit Daloze).


schneems (Richard Schneeman) wrote in #note-6:
> Instead of adding #line though if we could attach the source that would be more impactful for syntax search.
> 
> Some cases such as eval do not have source files, so if we could access the contents for casses without a path that would increase the capabilities of the gem.

Strongly agreed. We should have a way to get the original source code/text from an exception or a Method/UnboundMethod object, or any core object which has a `source_location`.
The current way trying to reread from disk is quite brittle and incomplete, as seen for `error_highlight` by @mame.
TruffleRuby already keeps this information, it's only a matter of exposing it through some method.
That should be a separate feature request though.

----------------------------------------
Feature #19138: `SyntaxError#path` for syntax_suggest
https://bugs.ruby-lang.org/issues/19138#change-100273

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
Currently syntax_suggest searches the path name from the exception message.
But extracting the info from messages for humans is fragile, I think.
So proposing a new method `SyntaxError#path`, similar to `LoadError#path`.

```patch
commit 986da132002af1cdb75c0c89ca2831fe51e6ce69
Author:     Nobuyoshi Nakada <nobu@ruby-lang.org>
AuthorDate: 2022-11-20 22:59:52 +0900
Commit:     Nobuyoshi Nakada <nobu@ruby-lang.org>
CommitDate: 2022-11-20 23:44:27 +0900

    Add `SyntaxError#path`

diff --git a/error.c b/error.c
index 0ff4b8d6d8e..ad1bc6ee8dc 100644
--- a/error.c
+++ b/error.c
@@ -125,6 +125,8 @@ err_vcatf(VALUE str, const char *pre, const char *file, int line,
     return str;
 }
 
+static VALUE syntax_error_with_path(VALUE, VALUE, VALUE*, rb_encoding*);
+
 VALUE
 rb_syntax_error_append(VALUE exc, VALUE file, int line, int column,
                        rb_encoding *enc, const char *fmt, va_list args)
@@ -138,15 +140,7 @@ rb_syntax_error_append(VALUE exc, VALUE file, int line, int column,
     }
     else {
         VALUE mesg;
-        if (NIL_P(exc)) {
-            mesg = rb_enc_str_new(0, 0, enc);
-            exc = rb_class_new_instance(1, &mesg, rb_eSyntaxError);
-        }
-        else {
-            mesg = rb_attr_get(exc, idMesg);
-            if (RSTRING_LEN(mesg) > 0 && *(RSTRING_END(mesg)-1) != '\n')
-                rb_str_cat_cstr(mesg, "\n");
-        }
+        exc = syntax_error_with_path(exc, file, &mesg, enc);
         err_vcatf(mesg, NULL, fn, line, fmt, args);
     }
 
@@ -2353,6 +2347,25 @@ syntax_error_initialize(int argc, VALUE *argv, VALUE self)
     return rb_call_super(argc, argv);
 }
 
+static VALUE
+syntax_error_with_path(VALUE exc, VALUE path, VALUE *mesg, rb_encoding *enc)
+{
+    if (NIL_P(exc)) {
+        *mesg = rb_enc_str_new(0, 0, enc);
+        exc = rb_class_new_instance(1, mesg, rb_eSyntaxError);
+        rb_ivar_set(exc, id_i_path, path);
+    }
+    else {
+        if (rb_attr_get(exc, id_i_path) != path) {
+            rb_raise(rb_eArgError, "SyntaxError#path changed");
+        }
+        VALUE s = *mesg = rb_attr_get(exc, idMesg);
+        if (RSTRING_LEN(s) > 0 && *(RSTRING_END(s)-1) != '\n')
+            rb_str_cat_cstr(s, "\n");
+    }
+    return exc;
+}
+
 /*
  *  Document-module: Errno
  *
@@ -3011,9 +3024,14 @@ Init_Exception(void)
     rb_eSyntaxError = rb_define_class("SyntaxError", rb_eScriptError);
     rb_define_method(rb_eSyntaxError, "initialize", syntax_error_initialize, -1);
 
+    ID id_path = rb_intern_const("path");
+
+    /* the path failed to parse */
+    rb_attr(rb_eSyntaxError, id_path, TRUE, FALSE, FALSE);
+
     rb_eLoadError   = rb_define_class("LoadError", rb_eScriptError);
     /* the path failed to load */
-    rb_attr(rb_eLoadError, rb_intern_const("path"), TRUE, FALSE, FALSE);
+    rb_attr(rb_eLoadError, id_path, TRUE, FALSE, FALSE);
 
     rb_eNotImpError = rb_define_class("NotImplementedError", rb_eScriptError);
 
```

With this method, syntax_suggest/core_ext.rb will no longer need `PathnameFromMessage`.

```patch
diff --git i/lib/syntax_suggest/core_ext.rb w/lib/syntax_suggest/core_ext.rb
index 40f5fe13759..616a6ed9839 100644
--- i/lib/syntax_suggest/core_ext.rb
+++ w/lib/syntax_suggest/core_ext.rb
@@ -25,15 +25,12 @@
       require "syntax_suggest/api" unless defined?(SyntaxSuggest::DEFAULT_VALUE)
 
       message = super
-      file = if highlight
-        SyntaxSuggest::PathnameFromMessage.new(super(highlight: false, **kwargs)).call.name
-      else
-        SyntaxSuggest::PathnameFromMessage.new(message).call.name
-      end
-
-      io = SyntaxSuggest::MiniStringIO.new
+      file = path
 
       if file
+        file = Pathname.new(file)
+        io = SyntaxSuggest::MiniStringIO.new
+
         SyntaxSuggest.call(
           io: io,
           source: file.read,
```

Since we have not released with `SyntaxError#detailed_message` yet, there should not be a compatibility issue.

@schneems How do you think?





-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/postorius/lists/ruby-core.ml.ruby-lang.org/

Thread

Prev Next

In This Thread

Prev Next