Errors #196

marcandre · 2020-11-07T15:24:43Z

A few files that generate errors with unparser, but have ok syntax according to ruby -c:

The text was updated successfully, but these errors were encountered:

mbj · 2020-11-09T01:40:11Z

A few files that generate errors with unparser, but have ok syntax according to ruby -c:

I've to go with ok syntax according to ruby-parser. Nevertheless there are valid cases there.

marcandre · 2020-11-09T01:44:58Z

A few files that generate errors with unparser, but have ok syntax according to ruby -c:

I've to go with ok syntax according to ruby-parser. Nevertheless there are valid cases there.

Yes, indeed. Maybe ruby-parse should have a -c option? In any case, apart from encoding issues I'd be curious of any cases of ruby-parser having troubles with any of these; they are all from very popular gems.

mbj · 2020-11-09T02:24:15Z

@marcandre All of them (past the earlier 2 you reported that where easy to fix) seem to be issues from the string / dstring madness.

Most of them are triggered by this edge case I intentionally unparsed "this way":

Original-Source:
'a\
b'

Generated-Source:
"a\\\n" "b"
Original-Node:
(dstr
  (str "a\\\n")
  (str "b"))
Generated-Node:
(dstr
  (str "a\\\n")
  (str "b"))
Success: x.rb

The problem is that there are AST constructs that are "only" reachable via dstring segments "segment-a" "segment-b" or via changing the delimiter to '.

The workaround to "create more segments to unparse dstrings" triggers invalid round trips (but same semantics so far) for the extended test cases you found.

It appears that this workaround to avoid "detecting the 3rd class of string flavor in the AST" may not be worth the hazzle.

So I may have to change unparser to detect ' delimiter to reduce the number of segments. Meh.

marcandre · 2020-11-09T02:38:18Z

It's not clear to me what the issue is, but I trust it should be possible to find an equivalent Ruby source code from the AST. Good luck 👍

mbj · 2020-11-09T02:44:26Z

It's not clear to me what the issue is, but I trust it should be possible to find an equivalent Ruby source code from the AST. Good luck

There is enough ruby code to match all ASTs. The challenge is supporting them all at once without going insane ;)

Let me try to explain it, as this activity will make it more clear for my mind.

Basically: I used to "cheat" on dstr nodes that do not have interpolation, and that cheat backfires with the examples you found.

This ruby file:

'a\
b'

which does NOT contain an interpolation, and does NOT represent an syntax level contatentation (as "foo" "bar" does), still generates a dstr in the AST:

(dstr
  (str "a\\\n")
  (str "b"))

I used to unparse all of "non intrpolating dynstrings" like the concatenation, including the case I cited above. And this "hack" (Which makes the code far easier) causes the issues you reported in this issue (which so far are 90% about string issues, where the string unparse would when evaluated be semantically equivalent but not at the AST level).

I basically have to undo this hack, and emit that string above in singlequotes (not syntax level concatenation) and than can set better segment brakes on literal (non interpolating) dynstrings that removes the issues.

mbj · 2020-11-23T22:18:45Z

@marcandre I've investigated almost all of the errors by now and they are all "dstring mangling" I talked about earlier.

I suspect it may be easier to change parser to produce "compact/minimized" dstrings instead of making unparser aware of all the quirks.

Talking over in the parser repository for possible AST improvements.whitequark/parser#765 (comment)

If the AST where only encoding semantics and not syntax leftovers the problems you reported here would disappear. I suspect its easier for me to contribute AST normalization to parser than to fix it on unparsers side.

iliabylich · 2020-11-23T22:24:59Z

Most of them are triggered by this edge case

Hmm, this is much harder to fix because that's what is emitted by the lexer:

bin/ruby-parse --30 -E test.rb
'a\
^ tSTRING_BEG "'"                               expr_end     [0 <= cond] [0 <= cmdarg]
'a\
 ^ tSTRING_CONTENT "a\\\n"                      expr_end     [0 <= cond] [0 <= cmdarg]
b'
^ tSTRING_CONTENT "b"                           expr_end     [0 <= cond] [0 <= cmdarg]
b'
 ^ tSTRING_END "'"                              expr_end     [0 <= cond] [0 <= cmdarg]
b'
  ^ tNL nil                                     line_begin   [0 <= cond] [0 <= cmdarg]

^ false "$eof"                                  line_begin   [0 <= cond] [0 <= cmdarg]
(dstr
  (str "a\\\n")
  (str "b"))

^ tSTRING_CONTENT is emitted twice. I believe it's a common rule of parser to emit multi-line strings line-by-line to ship source maps for line ends. Otherwise linters would have troubles in basic rules like line length validators.

mbj · 2020-11-23T22:27:54Z

@iliabylich I'm totally fine to use "consecutive" str nodes inside dstr as a hint for multi line / heredoc etc, even in the absence of interpolation.

But we could potentially collapse some of the odd cases (disussed above) that would flatten the mental complexity courve in unparser in the area already.

There are situations where parser right now produces different AST for the same semantics on dstrings. If we cut these cases down I'm fine.

iliabylich · 2020-11-23T22:33:54Z

I'm not sure if I understand the point of reducing AST for some cases when users still need all cases. Could you explain it please? Yes, it would be easier to support simple cases (because they'll have simpler AST), but to support 100% of AST patterns you need to handle complex AST anyway.

Also, (just out of curiosity) may I ask how do you perform unparsing by not looking at source maps of string literals? For example:

"foo'bar"
'foo"bar"
%q|foo'"bar|
%q{foo'"|bar}

Each string terminator is used to read string content in a very specific way (i.e. to treat chars that are not equal to terminator as a string content, " reads all chars except " etc) and at the same time there's no "universal" terminator.

iliabylich · 2020-11-23T22:36:07Z

There are situations where parser right now produces different AST for the same semantics on dstrings. If we cut these cases down I'm fine.

It definitely should be fixed if they are actually the same. Please, don't hesitate to create an issue 😄 I remember what you wrote about being busy, but we have a month, so it can wait.

mbj · 2020-11-23T23:02:03Z

I'm not sure if I understand the point of reducing AST for some cases when users still need all cases. Could you explain it please?

Simply managing the complexity budget. The core constraint of unparser is that it "can never look at source maps". And produce concrete syntax that when parsed: Returns the same AST.

If there is a group of ASTs that all represent the identical execution semantics, but can only be produced from different concrete syntax unparser has to special case, to select the concrete syntax that fits it. This is common for unparsers domain and expected.

The problem occurs if there group of becomes to-large. And the most complex group in parsers current AST format is dynamic strings.

The mental overhead to deal with the various edge cases grows exponentially. So if we can remove "one" special case, example %(); it may remove super linear complexity in unparser.

Each string terminator is used to read string content in a very specific way (i.e. to treat chars that are not equal to terminator as a string content, " reads all chars except " etc) and at the same time there's no "universal" terminator.

I had to remove the 2nd one as it had an unterminated string.

bundle exec unparser --verbose x.rb
x.rb
Original-Source:
"foo'bar"
%q|foo'"bar|
%q{foo'"|bar}

Generated-Source:
"foo'bar"
"foo'\"bar"
"foo'\"|bar"

Original-Node:
(begin
  (str "foo'bar")
  (str "foo'\"bar")
  (str "foo'\"|bar"))
Generated-Node:
(begin
  (str "foo'bar")
  (str "foo'\"bar")
  (str "foo'\"|bar"))
Success: x.rb

Unparser basically choses " as delimiter. And than re-escapes the payload. But it does not do this by itself it relies on rubies String#inspect to do this job.

I may emit strings without interpolation needs within ' in the future.

Edit: Basically for str unparsing is a simple as calling .inspect on that string ;) my problems are with dstr variance.

mbj · 2020-11-23T23:10:20Z

@iliabylich You have have misunderstood unparsers propose: Its NOT meant to reproduce your concrete syntax. Its only meant to reproduce concrete syntax that when parsed again produces the same AST.

The delimiters you gave me in an example above disappear in the AST, rightfully. As they have no execution semantics attached.

The issue I'm trying to put in writing is: There are too many ASTs in dynamic strings that have the same execution semantics but different concrete syntax. If we could remove just 10% of these complexity in unparser on dynamic strings drops significantly.

The ideal AST is: 1:1 execution semantics.

But dstr have a (too high for my pleasure) N:1 execution semantics. Reaching 1:1 is impossible (just an ideal) but reducing N is the key here.

Else the dstr is "to much" a source map by itself, and not an AST ;)

mbj · 2020-11-23T23:12:04Z

And just to be clear: I do not blame anyone for the current situation I describe as dstr variance. Making a free standing ruby parser was a big archievement for the ruby community. And I take it over any alternative. I'm just happy to see a way to reduce it in parser and ease my job with unparser.

iliabylich · 2020-11-23T23:19:21Z

I totally get it, no offense taken. I need exact code samples to talk about, and I agree that we should simplify them if possible

mbj · 2020-11-23T23:21:23Z

I'll get you a good list. I can source it from unparsers specs and simply commenting out special cases ;)

mbj · 2020-11-23T23:21:54Z

Also I'll not include multi line ones, as there is a good reason to have these for source maps.

mbj · 2024-06-22T16:10:34Z

@marcandre Its very likely all of these are fixed by #366 (not merged at the time of post). It changes the dstr node unparsing to an exhaustive search of all possible segment combinations, which is expensive but "guaranteed" to work.

I've also extended the corpus test there to include for example active_merchant for now. #366 will be merged soon once I fix the mutation coverage.

mbj mentioned this issue Nov 9, 2020

Multiple Errors #195

Open

mbj mentioned this issue Nov 23, 2020

+ ruby30.y: allow endless method without arglist. whitequark/parser#765

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Errors #196

Errors #196

marcandre commented Nov 7, 2020 •

edited

Loading

mbj commented Nov 9, 2020

marcandre commented Nov 9, 2020

mbj commented Nov 9, 2020 •

edited

Loading

marcandre commented Nov 9, 2020

mbj commented Nov 9, 2020 •

edited

Loading

mbj commented Nov 23, 2020

iliabylich commented Nov 23, 2020

mbj commented Nov 23, 2020

iliabylich commented Nov 23, 2020

iliabylich commented Nov 23, 2020

mbj commented Nov 23, 2020 •

edited

Loading

mbj commented Nov 23, 2020 •

edited

Loading

mbj commented Nov 23, 2020

iliabylich commented Nov 23, 2020

mbj commented Nov 23, 2020

mbj commented Nov 23, 2020

mbj commented Jun 22, 2024

Errors #196

Errors #196

Comments

marcandre commented Nov 7, 2020 • edited Loading

mbj commented Nov 9, 2020

marcandre commented Nov 9, 2020

mbj commented Nov 9, 2020 • edited Loading

marcandre commented Nov 9, 2020

mbj commented Nov 9, 2020 • edited Loading

mbj commented Nov 23, 2020

iliabylich commented Nov 23, 2020

mbj commented Nov 23, 2020

iliabylich commented Nov 23, 2020

iliabylich commented Nov 23, 2020

mbj commented Nov 23, 2020 • edited Loading

mbj commented Nov 23, 2020 • edited Loading

mbj commented Nov 23, 2020

iliabylich commented Nov 23, 2020

mbj commented Nov 23, 2020

mbj commented Nov 23, 2020

mbj commented Jun 22, 2024

marcandre commented Nov 7, 2020 •

edited

Loading

mbj commented Nov 9, 2020 •

edited

Loading

mbj commented Nov 9, 2020 •

edited

Loading

mbj commented Nov 23, 2020 •

edited

Loading

mbj commented Nov 23, 2020 •

edited

Loading