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

Show error cause reccursion information #13413

Closed
crowlKats opened this issue Jan 18, 2022 · 15 comments
Closed

Show error cause reccursion information #13413

crowlKats opened this issue Jan 18, 2022 · 15 comments
Labels
feat new feature (which has been agreed to/accepted) good first issue Good for newcomers

Comments

@crowlKats
Copy link
Member

Node does this
(code tested is https://github.com/denoland/deno/blob/167982be9e7af35e6c12ef6c40c002200bf5e0c0/cli/tests/testdata/error_cause_recursive.ts)
image
We should do the same

@crowlKats crowlKats added feat new feature (which has been agreed to/accepted) good first issue Good for newcomers labels Jan 18, 2022
@rushilmehra
Copy link

Going to attempt to pick this issue up. Will take me some time to familiarize myself with everything

@Grubba27
Copy link

Hello there! This issue is still open for contribuitons ? I would start by addeding another option for errors here at errors or somewhere else ? How could I test this ?
I have some implementation doubts also like, I'm able to always tell when this is happening ? or i would do this kind of message: "RangeError: Maximum call stack size exceeded"

@crowlKats
Copy link
Member Author

Hey @Grubba27, @rushilmehra is still working on this, but feel free to take a stab at this. We implemented this in the object inspection recently (#13555); obviously this wont map to rust, but it might still give some ideas.

I would start by addeding another option for errors here at errors or somewhere else ?

Take a look at #13209, we changed https://github.com/denoland/deno/blob/main/core/error.rs.

How could I test this ?

In the issue description i have linked a file with the testcase.

I have some implementation doubts also like, I'm able to always tell when this is happening ?

Not sure what you mean.

@luk3skyw4lker
Copy link

Is someone still working on this? I would like to grab this issue.

@rushilmehra
Copy link

Is someone still working on this? I would like to grab this issue.

I’m still taking a look, but I’ve been really busy at work. Feel free to take a stab.

@luk3skyw4lker
Copy link

Alright, I'll try something out and send the updates here, thanks!

@luk3skyw4lker
Copy link

@rushilmehra could you give some pointers later about where can I start? Only if it doesn't bothers you, I would appreciate it

@rushilmehra
Copy link

@rushilmehra could you give some pointers later about where can I start? Only if it doesn't bothers you, I would appreciate it

Take a look at @crowlKats comment earlier in the thread, it provides some good starting points. Feel free to study node’s implementation as well

@luk3skyw4lker
Copy link

Nice, thanks for the tip!

@Dgarc359
Copy link

Dgarc359 commented Apr 26, 2022

I'd be interested in taking a stab at this if there's still progress to be made. From a brief look at the source code, it looks like the target files for this fix would be error.rs or fmt_errors.rs

This is the output I see from running the test on your error_cause_recursive.ts
image

The notable differences being the missing trace that drills down to the object throwing the error. Would it require some changes to how sourcemap is used? Is the information that's missing in the linked image already available using the crates currently in use? Or does it require another, external, dependency?

Kind regards

@luk3skyw4lker
Copy link

I think that maybe only the error.rs and fmr_errrors.rs (together with 02_error.js that must be kept in sync with fmt_errrors.rs) should be the points of attention. I've looked at the code and the infos seem fine, but I think the formatting may be the most important.

I didn't manage to go through it yet because of work too, but feel free to take a stab.

@Cre3per
Copy link
Contributor

Cre3per commented Aug 2, 2022

Is someone still working on this? If not, I'd like to take over

I've got the sample test file covered

2022-08-02_16-02

Open points

  • A test for the sample file
  • Add <ref *> to non-top-most errors + Test
  • Handle aggregate errors + Test
  • Possibly sync with JavaScript. Not necessary so far.

Detecting circular references has proven challenging, because cause is a clone, rather than a reference as is in JavaScript. My current implementation copies JsError, except for the cause attribute, to form an ErrorIdentity. ErrorIdentity can then be compared to other ErrorIdentity to check for equality. This is memory intensive, but the best I could come up with so far. Help is welcome. in-dev: https://github.com/Cre3per/deno/blob/circular-cause-in-error/cli/fmt_errors.rs

@crowlKats I've looked at your circular detection for object inspection, thanks for the link! Am I right to assume that in this issue there is at most 1 circular reference, as the Error.cause chain is linear?

@Dgarc359
Copy link

Dgarc359 commented Aug 3, 2022

I couldn't get a solution working, feel free to take over

@Cre3per
Copy link
Contributor

Cre3per commented Sep 3, 2022

Feature is implemented in https://github.com/Cre3per/deno/tree/circular-cause-in-error
My branch has a failing test case (disabled) for when the recursively referenced error is not the top-most error. The test is currently failing due to #15602, I'll re-run the test case once that's fixed.

There's a decent chance I've missing some scenarios when AggregateError is involved. Considering node doesn't output causes for AggregateErrors at all, and AggregateError isn't all that common, I'd prefer to keep the implementation simple and not to look further into AggregateErrors.

@kamilogorek
Copy link
Contributor

Closed in #16384

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat new feature (which has been agreed to/accepted) good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

8 participants