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

refactor: Fix lints using clippy from nightly-2023-03-13 #6920

Merged
merged 19 commits into from Mar 14, 2023

Conversation

kdy1
Copy link
Member

@kdy1 kdy1 commented Feb 8, 2023

Description:

We may need to bump rkyv.
(#6807 (comment))

Related issue (if exists):

@kdy1 kdy1 added this to the Planned milestone Feb 8, 2023
@kdy1 kdy1 self-assigned this Feb 8, 2023
Copy link
Member Author

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

swc-bump:

  • dbg-swc

@kdy1 kdy1 marked this pull request as ready for review February 9, 2023 02:20
kodiakhq[bot]
kodiakhq bot previously approved these changes Feb 9, 2023
kodiakhq[bot]
kodiakhq bot previously approved these changes Feb 9, 2023
Copy link
Collaborator

@swc-bot swc-bot left a comment

Choose a reason for hiding this comment

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

Automated review comment generated by auto-rebase script

@kdy1 kdy1 disabled auto-merge February 9, 2023 03:46
@kdy1
Copy link
Member Author

kdy1 commented Feb 9, 2023

cc @kwonoj I think we find the condition for the UB

@kdy1 kdy1 marked this pull request as draft February 9, 2023 03:46
@kwonoj
Copy link
Member

kwonoj commented Feb 9, 2023

HUH, so old version of nightly was the reason?

@kdy1
Copy link
Member Author

kdy1 commented Feb 9, 2023

Not sure, but seems like one of two nightly versions are problem.
I think the newer version breaks rkyv, mainly because that's correct for UBs. i.e. A correct compiler optimization is added, but code with UB is broken by the optimization.

@kwonoj
Copy link
Member

kwonoj commented Feb 9, 2023

seems like one of two nightly versions are problem.
I think the newer version breaks rkyv, mainly because that's correct for UBs. i.e. A correct compiler optimization is added, but code with UB is broken by the optimization.

sounds quite scary, few random thinking

  • if a plugin uses certain version of compiler will it cause trouble even if swc/core have correct working compiler?
  • is there a way to check regressions like this?
  • in a long run, should we try to use stable compiler instead?

@kdy1
Copy link
Member Author

kdy1 commented Feb 9, 2023

if a plugin uses certain version of compiler will it cause trouble even if swc/core have correct working compiler?

Yes, single use is enough to break the plugin.

is there a way to check regressions like this?

Yes, that's why I added tests which invoke noop plugin with all AST we can parse, although it's not caught by it at this time.

#[testing::fixture("../swc_ecma_parser/tests/tsc/*.ts")]
#[testing::fixture("../swc_ecma_parser/tests/tsc/*.tsx")]

#[testing::fixture("../swc_css_parser/tests/fixture/**/input.css")]

in a long run, should we try to use stable compiler instead?

It's a bug of rkyv. For a compiler, UB is perfectly fine to break, in the name of optimization.
But I think we should consider changing serialization/deserialization method to something with less unsafe...

@kwonoj
Copy link
Member

kwonoj commented Feb 9, 2023

we should consider changing serialization/deserialization method to something with less unsafe.

Given wasmer itself uses rkyv as underlying serialization mechanism, I guess this'll require to take out wasmer entirely. Not impossible to do, but may need some considerations.

@kwonoj
Copy link
Member

kwonoj commented Feb 9, 2023

Forgot to mention, it looks like this occurred with serialization / deserialization of Program afaik. Something special occurs in those struct causes this?

@kdy1
Copy link
Member Author

kdy1 commented Feb 9, 2023

We can use other serialization/deserialization for Program, and let wasmer use rkyv.
I'm not sure which part is problematic.

           at rkyv::impls::core::<impl rkyv::DeserializeUnsized<[U],D> for [T]>::deserialize_unsized::h315b3a8965376629 (<module>[5344]:0x2b5bfa)
           at rkyv::impls::alloc::vec::<impl rkyv::Deserialize<alloc::vec::Vec<T>,D> for rkyv::vec::ArchivedVec<<T as rkyv::Archive>::Archived>>::deserialize::h04d1ec3ea3e970d8 (<module>[222]:0x13be0)
           at swc_ecma_ast::module_decl::_::<impl rkyv::Deserialize<swc_ecma_ast::module_decl::ImportDecl,__D> for <swc_ecma_ast::module_decl::ImportDecl as rkyv::Archive>::Archived>::deserialize::h185c61ae65c5c0b5 (<module>[5304]:0x2a51dd)
           at swc_ecma_ast::module_decl::_::<impl rkyv::Deserialize<swc_ecma_ast::module_decl::ModuleDecl,__D> for <swc_ecma_ast::module_decl::ModuleDecl as rkyv::Archive>::Archived>::deserialize::h952789b151c9a970 (<module>[5305]:0x2a597c)
           at swc_ecma_ast::module::_::<impl rkyv::Deserialize<swc_ecma_ast::module::ModuleItem,__D> for <swc_ecma_ast::module::ModuleItem as rkyv::Archive>::Archived>::deserialize::h88d8e5b435c0c70e (<module>[5724]:0x2cf7ff)
           at rkyv::impls::core::<impl rkyv::DeserializeUnsized<[U],D> for [T]>::deserialize_unsized::h006528daf2248e8c (<module>[5738]:0x2d413c)
           at rkyv::impls::alloc::vec::<impl rkyv::Deserialize<alloc::vec::Vec<T>,D> for rkyv::vec::ArchivedVec<<T as rkyv::Archive>::Archived>>::deserialize::h380c70590928e990 (<module>[227]:0x147f2)
           at swc_ecma_ast::module::_::<impl rkyv::Deserialize<swc_ecma_ast::module::Module,__D> for <swc_ecma_ast::module::Module as rkyv::Archive>::Archived>::deserialize::h86f80a54809dc175 (<module>[5721]:0x2ce741)
           at swc_ecma_ast::module::_::<impl rkyv::Deserialize<swc_ecma_ast::module::Program,__D> for <swc_ecma_ast::module::Program as rkyv::Archive>::Archived>::deserialize::he0c1e07561c3b81a (<module>[5723]:0x2cefcf)
           at swc_common::plugin::serialized::PluginSerializedBytes::deserialize::hd942b90ece579bfb (<module>[4013]:0x1ad338)
           at swc_common::plugin::serialized::deserialize_from_ptr::h3a43eb3fbcea6d36 (<module>[4012]:0x1ac686)
           at __transform_plugin_process_impl (<module>[6596]:0x31c0a3)
           at __transform_plugin_process_impl.command_export (<module>[11393]:0x45418b)

This is the clue, though.
So some array in ImportDecl is problematic, and it should be specifiers but not sure why it fails

@kwonoj
Copy link
Member

kwonoj commented Feb 9, 2023

I'd like to consider replacing serialization as a last resort, attempt to try fix to see if it works or not.

@kdy1
Copy link
Member Author

kdy1 commented Feb 9, 2023

Same here 👍

@kdy1
Copy link
Member Author

kdy1 commented Feb 9, 2023

I think rkyv is serializing data as ImportDecl even when it's not, so I left a comment.

rkyv/rkyv#338 (comment)

@kdy1 kdy1 force-pushed the rustc branch 2 times, most recently from 689412e to f5f35e4 Compare March 3, 2023 03:34
@kdy1 kdy1 marked this pull request as ready for review March 3, 2023 03:40
kodiakhq[bot]
kodiakhq bot previously approved these changes Mar 3, 2023
@kdy1 kdy1 enabled auto-merge (squash) March 3, 2023 03:40
@kdy1 kdy1 changed the title build: Update rustc to nightly-2023-03-13 refactor: Fix lints using clippy from nightly-2023-03-13 Mar 14, 2023
@kdy1 kdy1 marked this pull request as ready for review March 14, 2023 04:16
kodiakhq[bot]
kodiakhq bot previously approved these changes Mar 14, 2023
@kdy1 kdy1 merged commit 963c460 into swc-project:main Mar 14, 2023
@kdy1 kdy1 deleted the rustc branch March 14, 2023 04:56
@kdy1 kdy1 modified the milestones: Planned, v1.3.41 Mar 17, 2023
@swc-project swc-project locked as resolved and limited conversation to collaborators Apr 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants