Skip to content
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

Kill syntax error debugger #12910

Merged
merged 3 commits into from Mar 15, 2023

Conversation

privat
Copy link
Contributor

@privat privat commented Mar 6, 2023

Compilation errors happen, it's not a big deal.
Compilation clients are numerous, including highlighters and many source code related tools.

What is annoying is when an unrelated compilation error exception is fired (and not caught) by a random tool, and the human user is bothered by a buggy window asking them to fix something without context or anything.

Give me a real debugger!

So this PR remove all the useless SyntaxErrorDebugger code. Uncaught (or unexpected) SyntaxErrors will raise a classic debugger window, will all the context and information needed by users to fix (or report) the bug.

@Ducasse
Copy link
Member

Ducasse commented Mar 6, 2023

Yes :)

@privat
Copy link
Contributor Author

privat commented Mar 6, 2023

Maybe move SyntaxErrorNotification to be a subclass of Error instead of Notification? And rename it as SyntaxErrorError or simply SyntaxError?

@MarcusDenker
Copy link
Member

yes, after we get rid of the syntaxErrorDebugger, we can do that.

It is a Notification as we might get the syntaxErrorDebugger, fix the code, and continue.

@MarcusDenker
Copy link
Member

Yes, removing the #syntaxErrorDebugger is nice.

What is a bit sad is that we want to release Pharo11 quite soon... I wonder if this is the right time to untangle that mess.

@MarcusDenker
Copy link
Member

ZnHTTPSTest>>testGmailEncrypted times out on Mac and Linux

Did we not out a higher timeout for these at some point?

@svenvc
Copy link
Contributor

svenvc commented Mar 7, 2023

There is some mix-up: there are two identical classes: ZnHttpsTest and ZnHttpsTests.

Only the last one has a default time limit of 10 seconds.

This must have happened in the last merge.

@MarcusDenker
Copy link
Member

thanks, I added that to #12917

@MarcusDenker
Copy link
Member

One thing that now does not work anymore is loading code with syntax errors.

e.g. imagine a .st or .cs file, maybe from some other dialect or very old (e.g. with _ as assignments).

The idea was that with the syntax error debugger, one could, one by one, fix the error and load the code.

The removal now just opens a debugger, making it very hard to load the code.

One idea that I always wanted to do: load this kind of bad code with faulty mode. We could just enable
code gen for code with syntax errors for non-interactive compilations:

see:

@MarcusDenker
Copy link
Member

To experiment with faulty compilation, here for evaluate:

Go to OpalCompiler>>#evaluate and add the optionParseErrors line before compiling:

    ...
    self options:  #(+ optionParseErrors).
    doItMethod := self compile.
    ...

If you now run

Smalltalk compiler evaluate: '1+'

it opens the normal debugger, but on the wrong doit

49962154-3ab8-4814-ba98-d33d5142d07d

What does not yet work

  • edit-and-continue, the debugger does not allow it (but that can be fixed)
  • not the best visualisation of the error position, but for this case I think it is good enough

@privat
Copy link
Contributor Author

privat commented Mar 7, 2023

One thing that now does not work anymore is loading code with syntax errors.
e.g. imagine a .st or .cs file, maybe from some other dialect or very old (e.g. with _ as assignments).
The idea was that with the syntax error debugger, one could, one by one, fix the error and load the code.

I think this is a design issue. Error recovery when loading random source code if a neat feature, but maybe it should be done by the loader tool. Not as an intrinsic feature of the image.

Crippling the UI because some experimental code highlighter or another tool throws a syntax error make little sense. I had once a syntax error thrown at me after saving the image: because the AST cache was cleaned, and a background Calypso window was open with a method with faulty experimental code, so Calypso naturally wanted to get a source code to present, and for some reason the decompiler was called, and after the decompiler did its decompilation it went for some semantic analysis on it, that thrown a error. It was weird.

Moreover, the original design is just BAD: recovering errors ONE by ONE in the order decided by the loader seems insane, as the user is let without context nor understanding nor even a precisely defined state (what is already loaded?). For instance, a better loader could do a plan of the change, check that the plan and the data is consistent, try to improve the plan if not consistent (asking the user on the whole thing with all information within a consistent state), then apply the plan. Asking the loader to trust the image for 'syntax error recovery' remove the control on the tool.

Note that an alternative (aggressive) loader design could be to just install faulty code, then present to the user a list of all things to fix as a somewhat post-loading task (or let the user YOLO and execute the code anyway, no need to fix something you do not plan to use but was loaded anyway in the package).

@privat
Copy link
Contributor Author

privat commented Mar 7, 2023

To experiment with faulty compilation, here for evaluate: [...]

I obviously did experiment a lot with faulty compilation, since it's basically the point of the whole "improve faulty compilation" series of PR. RBCodeSnippet have a testExecute whose main point was to deal with faulty methods and expressions. https://github.com/pharo-project/pharo/blob/Pharo11/src/OpalCompiler-Tests/RBCodeSnippetTest.extension.st#L76

However, I did not really try to use it and try to recover from errors in day-to-day tasks. Maybe I should just activate it globally :)

GitHub
Pharo is a dynamic reflective pure object-oriented language supporting live programming inspired by Smalltalk. - pharo/RBCodeSnippetTest.extension.st at Pharo11 · pharo-project/pharo

@privat
Copy link
Contributor Author

privat commented Mar 7, 2023

What is a bit sad is that we want to release Pharo11 quite soon... I wonder if this is the right time to untangle that mess.

You guys should try yo communicate better about release dates and process. Maybe having some kind of feature freeze date so devs can plan things acordingly?

@MarcusDenker
Copy link
Member

I was thinking about this a bit and I now think that yes, we should do it.

  • loading broken .st and .cs files will be easily possible with making faulty parsing the default in non-interactive mode
  • the only thing we have to do is to guard git commits, and that is not hard to do

@MarcusDenker
Copy link
Member

Maybe better for the Pharo 12 branch (this way we can really clean up and break things for a while..)

@privat privat changed the base branch from Pharo11 to Pharo12 March 14, 2023 13:56
@privat
Copy link
Contributor Author

privat commented Mar 14, 2023

Let's merge all the things \o/

@jecisc
Copy link
Member

jecisc commented Mar 14, 2023

Currently the P12 branch is not building but I'm hoping to fix that with: #13004

@tesonep tesonep added this to the 12.0.0 milestone Mar 15, 2023
@privat privat changed the base branch from Pharo12 to Pharo11 March 15, 2023 19:07
@privat privat changed the base branch from Pharo11 to Pharo12 March 15, 2023 19:07
@MarcusDenker MarcusDenker merged commit 53cbf72 into pharo-project:Pharo12 Mar 15, 2023
3 checks passed
@privat privat mentioned this pull request Apr 13, 2023
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants