Skip to content

Conversation

@schneems
Copy link
Collaborator

Display raw errors from Ruby's parser

Currently, we're trying to make sense of the errors by the whole document and delivering one "banner" stating the cause of the problem. This is based on the "who dis" ripper subclass which only captures one error.

This causes problems, some described in #68 (comment)

Instead we can remove the banner and rely on the parser's error messages for the short term. With this move, we're going to multi errors per output and not attempting to re-explain the existing ripper error. The main focus is showing the problem rather than explaining it.

Example:

$ ./exe/dead_end spec/fixtures/rexe.rb.txt
--> /Users/rschneeman/Documents/projects/dead_end/spec/fixtures/rexe.rb.txt

syntax error, unexpected `def', expecting ')'
syntax error, unexpected `end', expecting end-of-input

   16  class Rexe
❯  40    class Options < Struct.new(
❯  41        :input_filespec,
❯  42        :input_format,
❯  43        :input_mode,
❯  44        :loads,
❯  45        :output_format,
❯  46        :output_format_tty,
❯  47        :output_format_block,
❯  48        :requires,
❯  49        :log_format,
❯  71    end
  551  end

syntax error, unexpected end-of-input, expecting `end'

   16  class Rexe
   40    class Options < Struct.new(
   71    end
❯  77    class Lookups
❯ 140      def format_requires
❯ 148    end
  551  end

Eventually, We can re-work this logic to attempt to do a better job at explaining errors based on lex values in each captured block rather than using regex values on the parser errors.

[Breaking] Remove previously deprecated require "dead_end/fyi" interface && Move dead_end/internals to dead_end

The dead_end/fyi file was created so that there could be an "fyi" interface that told people to run the CLI command, as well as an "auto" interface that fired the search functionality.

I did this for safety reasons originally. Since then, the reliability has increased. We have deprecated and then removed the dead_end/fyi interface and can now also get rid of this internals file. That allows us to have the core functionality where you would expect it.

DeadEnd is now fired on EVERY syntax error

When I created dead_end I wanted to be very cautious. It has proved to be very durable and dependable. We no longer have to gate only errors with end-of-input in it.

Currently we're trying to make sense of the errors by the whole document and delivering one "banner" stating the cause of the problem.

This is based on the "who dis" ripper subclass which only captures one error.

With this move, we're going to multi errors per output and not attempting to re-explain the existing ripper error. The main focus is showing the problem rather than explaining it.

Example:

```
$ ./exe/dead_end spec/fixtures/rexe.rb.txt
--> /Users/rschneeman/Documents/projects/dead_end/spec/fixtures/rexe.rb.txt

syntax error, unexpected `def', expecting ')'
syntax error, unexpected `end', expecting end-of-input

   16  class Rexe
❯  40    class Options < Struct.new(
❯  41        :input_filespec,
❯  42        :input_format,
❯  43        :input_mode,
❯  44        :loads,
❯  45        :output_format,
❯  46        :output_format_tty,
❯  47        :output_format_block,
❯  48        :requires,
❯  49        :log_format,
❯  71    end
  551  end

syntax error, unexpected end-of-input, expecting `end'

   16  class Rexe
   40    class Options < Struct.new(
   71    end
❯  77    class Lookups
❯ 140      def format_requires
❯ 148    end
  551  end
```

Eventually We can re-work this logic to attempt to do a better job at explaining errors based on lex values in each captured block rather than using regex values on the parser errors.
This file was created so that there could be an "fyi" interface that told people  to run the CLI command, as well as a "auto" interface that fired the search functionality.

I did this for safety reasons originally. Since then, the reliability has increased. We have deprecated and then removed the `dead_end/fyi` interface and can now also get rid of this internals file. That allows us to have the core functionality where you would expect it.
This class was used for detecting the syntax error type and emitting a "better" error than the parser. It made several incorrect assumptions, mainly that the document would only have a single syntax error.

Over time this has proven to be an ineffective if not outright confusing annotation. With all the usage removed, these classes can also be removed.

Ideally we will come back and add a better, more robust way to improve the errors, at which time we can revisit the banner code which will continue to live on his git history.
@schneems schneems force-pushed the schneems/display-each-block branch from 9c77cc6 to 3ca6da8 Compare October 31, 2021 03:08
@schneems
Copy link
Collaborator Author

schneems commented Oct 31, 2021

In moving to annotating syntax errors per section/block and using the Ruby parse error output I am realizing we've got a problem. Take a look at this output:

$ ./exe/dead_end spec/fixtures/rexe.rb.txt
--> /Users/rschneeman/Documents/projects/dead_end/spec/fixtures/rexe.rb.txt

syntax error, unexpected `def', expecting ')'
syntax error, unexpected `end', expecting end-of-input

   16  class Rexe
❯  40    class Options < Struct.new(
❯  41        :input_filespec,
❯  42        :input_format,
❯  43        :input_mode,
❯  44        :loads,
❯  45        :output_format,
❯  46        :output_format_tty,
❯  47        :output_format_block,
❯  48        :requires,
❯  49        :log_format,
❯  71    end
  551  end

syntax error, unexpected end-of-input, expecting `end'

   16  class Rexe
   40    class Options < Struct.new(
   71    end
❯  77    class Lookups
❯ 140      def format_requires
❯ 148    end
  551  end

The syntax error output comes from the code in the block, which is why it says "expecting end-of-input" and "unexpected end-of-input" even though that's not the syntax error the end-user would see.

I think that might be confusing to the user. I've got some ideas for fixing it:

Options

  • Amend with something like [in section] to indicate it's based on just the code shown:
syntax error, unexpected `end', expecting end-of-input [in section]

# and

syntax error, unexpected end-of-input [in section], expecting `end'

Consequences: It clears up the ambiguity a bit, but it makes the message longer. It might also be confusing what "in section" means. Possibly there's better wording. The brackets make it clearer that the output is modified, but also might be confusing since it's not a typical feature found in syntax errors otherwise.

  • Gsub out those parts:
syntax error, unexpected `end'

# and

syntax error, expecting `end'

Consequences: It clears up the ambiguity for these two cases, it might make less sense for other end-of-input output, it's hard to know. I don't like that we're not leaving behind any trace of what was removed, we could perhaps replace it with something like [...] but that syntax might also be confusing.

  • Do nothing and leave it.

Consequences: It might be confusing to some developers, other developers might not even notice it. The current output messages aren't always correct and the output is still useful.

  • Other ideas?

Feedback

@mauro-oto, @BuonOmo, @olleolleolle, @keithrbennett, @stoivo I'm curious what you all think about this output, and the problem described here? If you've got some time, I would love some feedback.

@BuonOmo
Copy link
Contributor

BuonOmo commented Oct 31, 2021

IMHO, the Gsub out those parts is the clearer. I would just like to be hinted what I have to add, and then I'd look at the simplified file to see where I should add the missing end.

However, I'm still having a (quite) hard time reading the simplified files, yet IDK how I could feel more comfortable with it for now. Anyway I think the examples you gave here are readable, and would benefit a simpler message to avoid confusion.

@stoivo
Copy link
Contributor

stoivo commented Oct 31, 2021

I dont completely understand the changes, so ¯_(ツ)_/¯ propabaly ok

@schneems
Copy link
Collaborator Author

schneems commented Nov 1, 2021

Thanks for the feedback! I went ahead and pushed through with getting the lexical banner working rather than shipping this intermediate fix (since i think that output directly from the parser is still not great).

I'm going to merge this for now, and then I've got another PR adding the banner/lex logic in.

@schneems schneems merged commit ceadc9a into main Nov 1, 2021
@schneems schneems deleted the schneems/display-each-block branch November 1, 2021 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants