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

feat(stdlibs/errors): support Is function #1929

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

notJoon
Copy link
Member

@notJoon notJoon commented Apr 15, 2024

Description

Added the Is function to the errors package.

  • Implemented it based on the errorString type
  • Checks if the error type and message are the same
  • Uses Unwrap method to traverse the error chain to find an error that matches the target error.

Relative Issue

#486 // Failed to implement As function due to the technical issue. maybe later

@notJoon notJoon requested review from jaekwon and thehowl as code owners April 15, 2024 08:28
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Apr 15, 2024
Copy link

codecov bot commented Apr 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.96%. Comparing base (831bb6f) to head (d605d2a).
Report is 529 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1929      +/-   ##
==========================================
- Coverage   47.74%   45.96%   -1.79%     
==========================================
  Files         393      483      +90     
  Lines       61629    69533    +7904     
==========================================
+ Hits        29424    31959    +2535     
- Misses      29734    34927    +5193     
- Partials     2471     2647     +176     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@notJoon notJoon changed the title feat(gnovm): Add errors.Is function feat(gnovm): Support Is function in the errors package Apr 15, 2024
@harry-hov harry-hov changed the title feat(gnovm): Support Is function in the errors package feat(stdlibs/errors): support Is function Apr 23, 2024
harry-hov
harry-hov previously approved these changes Apr 23, 2024
@harry-hov harry-hov self-requested a review April 23, 2024 16:47
Comment on lines +104 to +122
for {
if err == target {
return true
}

if x, ok := err.(interface{ Is(error) bool }); ok && x.Is(target) {
return true
}

if x, ok := err.(*errorString); ok {
if t, ok := target.(*errorString); ok && x.s == t.s {
return true
}
}

if err = Unwrap(err); err == nil {
return false
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried about edge cases here. Can it go infinite?

Copy link
Member Author

@notJoon notJoon Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think an infinity loop will be caused when trying to unwrap the circular referenced errors. this can be prevented by just tracing whether the error has been visited or not.

return e.msg == t.msg
}

func TestIs(t *testing.T) {
Copy link
Contributor

@harry-hov harry-hov Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have same set of tests as errors/wrap.go:L16-69?

@harry-hov harry-hov dismissed their stale review April 23, 2024 16:59

should be approved by someone with better understanding

@notJoon
Copy link
Member Author

notJoon commented Apr 24, 2024

For this code to fully work, the interface type of gno2GoType needs to be full supported. I'll leave it as a draft until that is resolved.

https://go.dev/play/p/XUDNrojJ2jh

case *InterfaceType:
if ct.IsEmptyInterface() {
// XXX move out
rt := reflect.TypeOf((*interface{})(nil)).Elem()
return rt
} else {
// NOTE: can this be implemented in go1.15? i think not.
panic("not yet supported")
}

@notJoon notJoon marked this pull request as draft April 24, 2024 02:53
Copy link

This PR is stale because it has been open 3 months with no activity. Remove stale label or comment or this will be closed in 3 months.

@github-actions github-actions bot added Stale and removed Stale labels Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: In Progress
Status: No status
Status: Triage
Development

Successfully merging this pull request may close these issues.

2 participants