Skip to content

webassembly/objpyproxy: Avoid throwing on implicit symbols access.#17683

Merged
dpgeorge merged 1 commit intomicropython:masterfrom
WebReflection:issue-17657
Jul 17, 2025
Merged

webassembly/objpyproxy: Avoid throwing on implicit symbols access.#17683
dpgeorge merged 1 commit intomicropython:masterfrom
WebReflection:issue-17657

Conversation

@WebReflection
Copy link
Copy Markdown
Contributor

@WebReflection WebReflection commented Jul 15, 2025

Summary

This MR fixes #17657 by guarding against implicit unknown symbols accessed directly by specific JS native APIs.

Testing

Added a new get proxy trap focused test that would throw otherwise with current MicroPython when Object.prototype.toString.call(pyProxy) happens, returning null just like the then case would for any symbol that is not Symbol.iterator.

Trade-offs and Alternatives

The typeof operator is the fastest check JS offers for primitive types so there are not many side-effects or trafe-offs in here, just better runtime handling of unexpected symbol access that both 3rd party libraries or native JS APIs would regularly perform.

The only alternative approach is to return undefined which could be returned for then case too but neither of these actually matter as both then and Symbol.toStringTag or others are implicit via Reflect so that returning null is still OK, yet if anyone checks strictly against undefined that might fail but it's a very edge/uncommon use case to consider, so that changes in here are minimal.

@WebReflection WebReflection force-pushed the issue-17657 branch 2 times, most recently from e68d2fd to d4503a4 Compare July 15, 2025 08:49
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 15, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.44%. Comparing base (554f114) to head (c72a3e5).
Report is 1 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master   #17683   +/-   ##
=======================================
  Coverage   98.44%   98.44%           
=======================================
  Files         171      171           
  Lines       22208    22208           
=======================================
  Hits        21863    21863           
  Misses        345      345

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

dpgeorge
dpgeorge reviewed Jul 15, 2025
View reviewed changes
@dpgeorge
Copy link
Copy Markdown
Member

dpgeorge commented Jul 15, 2025

The only alternative approach is to return undefined

I also thought this might be better, since it matches the JS side, eg:

$ node
> {'0':1}[Symbol.toStringTag]
undefined

But I guess null is acceptable as your reasoning shows.

@WebReflection
Copy link
Copy Markdown
Contributor Author

WebReflection commented Jul 15, 2025

But I guess null is acceptable as your reasoning shows.

if I change null in there I gotta change null for then too as that also reflects JS better and I am not sure why it was null in the first place?

$ node
> ({'0':1}.then)
undefined

@WebReflection
Copy link
Copy Markdown
Contributor Author

WebReflection commented Jul 15, 2025 •
edited
Loading

@dpgeorge in latest push we have:

  • _ref still with fast/priority access
  • then and symbols aggregated as fallback for undefined (instead of null) ... an extra check is done for strict equality check against Symbol.iterator
  • more readable test that explicitly access an unknown symbol and also checks against then (I think that was a nice to have)

anything else I should change?

edit ... well, I could just return; instead of return undefined; ... I don't think it matters and maybe for readability sake (or "greppability") that return undefined; is just fine?

dpgeorge
dpgeorge approved these changes Jul 15, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@dpgeorge dpgeorge left a comment

Choose a reason for hiding this comment

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

This looks good now, thanks for updating!

@dpgeorge
Copy link
Copy Markdown
Member

dpgeorge commented Jul 15, 2025

I don't think it matters and maybe for readability sake (or "greppability") that return undefined; is just fine?

return undefined is better, especially if you're not used to JS like me :)

@WebReflection @dpgeorge
webassembly/objpyproxy: Avoid throwing on implicit symbols access.
c72a3e5 
This commit improves get handling by guarding against implicit unknown
symbols accessed directly by specific JS native APIs.

Fixes issue micropython#17657.

Signed-off-by: Andrea Giammarchi <[email protected]>
@dpgeorge dpgeorge merged commit c72a3e5 into micropython:master Jul 17, 2025
31 checks passed
@WebReflection
Copy link
Copy Markdown
Contributor Author

WebReflection commented Jul 17, 2025

thanks @dpgeorge , any chance this can make it to npm too?

@dpgeorge
Copy link
Copy Markdown
Member

dpgeorge commented Jul 17, 2025

any chance this can make it to npm too?

Yes, I'll do that soon.

@dpgeorge
Copy link
Copy Markdown
Member

dpgeorge commented Jul 18, 2025

Now published to npmjs: https://www.npmjs.com/package/@micropython/micropython-webassembly-pyscript/v/1.26.0-preview-386

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dpgeorge dpgeorge dpgeorge approved these changes

Assignees

No one assigned

Labels

port-webassembly

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

webassembly: getting JS symbols implicitly also throws errors

2 participants

@WebReflection @dpgeorge