Project

General

Profile

Actions

Feature #19138

closed

`SyntaxError#path` for syntax_suggest

Added by nobu (Nobuyoshi Nakada) over 1 year ago. Updated over 1 year ago.

Status:
Closed
Assignee:
-
Target version:
[ruby-core:110828]

Description

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.

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.

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 (Richard Schneeman) How do you think?

Actions #1

Updated by nobu (Nobuyoshi Nakada) over 1 year ago

  • Description updated (diff)
Actions #2

Updated by nobu (Nobuyoshi Nakada) over 1 year ago

  • Tracker changed from Bug to Feature
  • Backport deleted (2.7: DONTNEED, 3.0: DONTNEED, 3.1: DONTNEED)

Updated by Eregon (Benoit Daloze) over 1 year ago

Should we also expose the line. Or maybe source_location?
Just a suggestion, if we only need path I think it's fine to just add SyntaxError#path.

Updated by nobu (Nobuyoshi Nakada) over 1 year ago

SyntaxError can have a series of errors.
Before adding line, we need to consider how to provide that list.

Updated by schneems (Richard Schneeman) over 1 year ago

I love the idea.

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.

Updated by Eregon (Benoit Daloze) over 1 year ago

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 (Yusuke Endoh).
TruffleRuby already keeps this information, it's only a matter of exposing it through some method.
That should be a separate feature request though.

Actions #8

Updated by hsbt (Hiroshi SHIBATA) over 1 year ago

  • Target version set to 3.2

Updated by matz (Yukihiro Matsumoto) over 1 year ago

Sounds reasonable.

Matz.

Actions #10

Updated by nobu (Nobuyoshi Nakada) over 1 year ago

  • Status changed from Open to Closed

Applied in changeset git|4e68b594314760611d0926c3de70a4cad26802cd.


[Feature #19138] Add SyntaxError#path

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0