feat(#5166): safe navigation and on-error handling#5176
Conversation
🚀 Performance AnalysisAll benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information. Click to see the detailed report
✅ Performance gain: |
|
@akrzywicki Thanks for your contribution, we appreciate it! Before we can merge this pull request, we need to see all CI workflows pass without errors. We can't merge the pull request into the master branch unless all workflows are green. Try to fix them, also paying attention to code style issues. |
f2baaca to
6960fdd
Compare
|
@maxonfjvipon WDYT? |
maxonfjvipon
left a comment
maxonfjvipon
left a comment
There was a problem hiding this comment.
@akrzywicki not enough tests with longer dispatch/application chains in runtime and in parser
| "Couldn't create a Win32 socket, WSA error code: %d" | ||
| * err.code | ||
| try | ||
| on-error |
There was a problem hiding this comment.
@akrzywicki just replacing try with new object is not what we wanted to achieve, I don't think we need some extra object to replace try at all
There was a problem hiding this comment.
@maxonfjvipon Thanks for the review.
I see, #5166 is about the new error-handling model (book?, ?., desugar), not a straight try -> on-error rename across stdlib.
In this PR I used on-error mainly where the new pipeline needs it (desugar target and dataized). The explicit on-error blocks in stdlib were my attempt to remove legacy try step by step - if that is not the direction you want, I will adjust the approach.
I can revert the stdlib migration (and bring try back there if needed) and keep on-error only as the desugar/runtime target plus dataized. Or follow another pattern you prefer.
What would you like me to do?
| <xsl:function name="eo:safe-method"> | ||
| <xsl:param name="base"/> | ||
| <xsl:param name="mtd"/> | ||
| <xsl:text>DataizedSafe.take(</xsl:text> |
There was a problem hiding this comment.
@akrzywicki this safe dataization should not be exposed in the generated code, we've already have PhSafe
There was a problem hiding this comment.
Addressed: generated code now uses PhSafe.takeNav instead of DataizedSafe.take. DataizedSafe remains internal for tests and dataized() helper.
|
@maxonfjvipon Re: longer dispatch/application chains - added:
Happy to extend if you want more cases. |
6960fdd to
6c2a620
Compare
Replace legacy try with on-error, desugar book? to Φ.on-error, and emit DataizedSafe for ?. at transpile time so dataization errors become values.
6c2a620 to
dae0eec
Compare
|
|
@akrzywicki merge conflicts here |
3 participants
Summary
?.safe navigation: parser@safe, transpile viaDataizedSafe.take, runtime catchesEOerror.ExErrorand returns the error object as a value.book?on-error replacement: parser@on-error, canonical lowering inexpand-on-error.xsltoΦ.on-errorapplication.trywithon-error(on-error.eo,EOonError), migrate stdlib.eocall sites, removetry.eo/EOtry.Known gaps / follow-ups
book? > [e] >>(issue example withtt.sprintfon following lines) is not parsed yet; documented inPARSER_SPEC.mdas a follow-up. Desugar covers the only-phi handler shape covered by eo-syntax packs.Data.ToPhi/ExReadOnlyregression after pure objects #5165 (PhApplication/PhOnce): constructingData.ToPhiwhile anAtOnce(AtComposite)attribute is being read can throwExReadOnly. New JUnit tests avoidToPhiin attribute getters; some existing tests (e.g.EOerrorTest) may still fail on currentmasterlineage until that interaction is fixed upstream or in a separate PR.Test plan
mvn test -pl eo-parser -Dtest=EoSyntaxTestmvn test -pl eo-parser -Dtest=EoTest#parsesSafeNavigationInChain,EoTest#parsesOnErrorOnlyPhimvn test -pl eo-runtime -Dtest=DataizedSafeTest,EOonErrorTestmvn test -pl eo-maven-plugin -Dtest=MjTranspileTest(includessafe-navigationpack)rm -rf ~/.eo/transpiled && mvn clean install -Pqulice(maintainer / CI; may surfaceData.ToPhifailures unrelated to this diff)Fixes #5166