Skip to content

Upgrade tsify version - #2534

Open
katherine-hough wants to merge 3 commits into
cedar-policy:mainfrom
katherine-hough:tsify
Open

Upgrade tsify version#2534
katherine-hough wants to merge 3 commits into
cedar-policy:mainfrom
katherine-hough:tsify

Conversation

@katherine-hough

Copy link
Copy Markdown
Contributor

Description of changes

tsify released a fix for the renaming issue we encountered back in November enabling us to upgrade to the latest version using the new rename container attribute.

However, the into_wasm_abi/from_wasm_abi
container attributes which we heavily use are now deprecated because they can cause memory leaks. The recommended alternative is to use the Ts wrapper type. To avoid changing the signature of the existing public Rust functions I made new wasm variants. (e.g., pub fn is_authorized_wasm(call: Ts<AuthorizationCall>) -> Result<Ts<AuthorizationAnswer>, JsError>).

To check backwards compatibility of the Rust API, I used cargo expand to diff the code for cedar-policy before and after this change. When every feature except tsify, wasm, and wasm-bindgen were enabled there is no difference.
When all features are enabled there are two main differences that are not backwards compatible:

  1. There are no longer derived implementations of OptionFromWasmAbi, FromWasmAbi, OptionIntoWasmAbi, IntoWasmAbi and WasmDescribe for types previously annotated with #[tsify(into_wasm_abi, from_wasm_abi)].
  2. The signature of bindgen-generated functions are different
    Main:
            pub unsafe extern "C-unwind" fn __wasm_bindgen_generated_checkParseEntities(
                arg0_1: <<EntitiesParsingCall as wasm_bindgen::convert::FromWasmAbi>::Abi as wasm_bindgen::convert::WasmAbi>::Prim1,
                arg0_2: <<EntitiesParsingCall as wasm_bindgen::convert::FromWasmAbi>::Abi as wasm_bindgen::convert::WasmAbi>::Prim2,
                arg0_3: <<EntitiesParsingCall as wasm_bindgen::convert::FromWasmAbi>::Abi as wasm_bindgen::convert::WasmAbi>::Prim3,
                arg0_4: <<EntitiesParsingCall as wasm_bindgen::convert::FromWasmAbi>::Abi as wasm_bindgen::convert::WasmAbi>::Prim4,
            ) -> wasm_bindgen::convert::WasmRet<
                <CheckParseAnswer as wasm_bindgen::convert::ReturnWasmAbi>::Abi,
            > {  }
}

This change:

        pub unsafe extern "C-unwind" fn __wasm_bindgen_generated_checkParseEntities(
                arg0_1: <<Ts<
                    EntitiesParsingCall,
                > as wasm_bindgen::convert::FromWasmAbi>::Abi as wasm_bindgen::convert::WasmAbi>::Prim1,
                arg0_2: <<Ts<
                    EntitiesParsingCall,
                > as wasm_bindgen::convert::FromWasmAbi>::Abi as wasm_bindgen::convert::WasmAbi>::Prim2,
                arg0_3: <<Ts<
                    EntitiesParsingCall,
                > as wasm_bindgen::convert::FromWasmAbi>::Abi as wasm_bindgen::convert::WasmAbi>::Prim3,
                arg0_4: <<Ts<
                    EntitiesParsingCall,
                > as wasm_bindgen::convert::FromWasmAbi>::Abi as wasm_bindgen::convert::WasmAbi>::Prim4,
            ) -> wasm_bindgen::convert::WasmRet<
                <Result<
                    Ts<CheckParseAnswer>,
                    JsError,
                > as wasm_bindgen::convert::ReturnWasmAbi>::Abi,
            > {  }

To check the backwards compatibility of the TypeScript API, I built the WASM bindings using the "build-wasm.sh" script on this change and main. I manually inspected the diffs. The type definitions and signatures appear to be the same. This is consistent with the tsify documentation which says that the generated TypeScript is unchanged when changing to the Ts wrappers.

Issue #, if available

Resolves: #1744

Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

  • A bug fix or other functionality change requiring a patch to cedar-policy.

I confirm that this PR (choose one, and delete the other options):

  • Updates the "Unreleased" section of the CHANGELOG with a description of my change (required for major/minor version bumps).

I confirm that cedar-spec (choose one, and delete the other options):

  • Does not require updates because my change does not impact the Cedar formal model or DRT infrastructure.

I confirm that docs.cedarpolicy.com (choose one, and delete the other options):

  • Does not require updates because my change does not impact the Cedar language specification.

Signed-off-by: Katherine Hough <kmhough@amazon.com>
Signed-off-by: Katherine Hough <kmhough@amazon.com>
@github-actions

Copy link
Copy Markdown

Coverage Report

Head Commit: 00e5cd298e1121f59aea39bb7f3c76e524ca58cd

Base Commit: b8fc36d497501d97b8be977d16ffed94b76a89b5

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 0.00%

Status: FAILED ❌

Details
File Status Covered Coverage Missed Lines
cedar-policy/src/ffi/check_parse.rs 🔴 0/18 0.00% 43-47, 89-91, 196-200, 251-255
cedar-policy/src/ffi/convert.rs 🔴 0/28 0.00% 42-46, 88-90, 112-114, 136-138, 163-165, 190-192, 231-233, 274-278
cedar-policy/src/ffi/format.rs 🔴 0/3 0.00% 44-46
cedar-policy/src/ffi/is_authorized.rs 🔴 0/25 0.00% 66-68, 126-131, 163-168, 202-206, 681-685
cedar-policy/src/ffi/validate.rs 🔴 0/3 0.00% 47-49
cedar-wasm/src/utils.rs 🔴 0/12 0.00% 35-40, 64-69

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 88.22%

Status: PASSED ✅

Details
Package Status Covered Coverage Base Coverage
cedar-language-server 🟢 4722/5102 92.55% --
cedar-policy 🟢 4849/6027 80.45% --
cedar-policy-cli 🟡 1294/1669 77.53% --
cedar-policy-core 🟢 24673/27843 88.61% --
cedar-policy-formatter 🟢 914/1088 84.01% --
cedar-policy-symcc 🟢 6930/7405 93.59% --
cedar-wasm 🔴 0/40 0.00% --

@github-actions

Copy link
Copy Markdown

Coverage Report

Head Commit: bf7982db53ca9048c56fc416fe340c445717fa4f

Base Commit: b8fc36d497501d97b8be977d16ffed94b76a89b5

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 0.00%

Status: FAILED ❌

Details
File Status Covered Coverage Missed Lines
cedar-policy/src/ffi/check_parse.rs 🔴 0/18 0.00% 43-47, 89-91, 196-200, 251-255
cedar-policy/src/ffi/convert.rs 🔴 0/28 0.00% 42-46, 88-90, 112-114, 136-138, 163-165, 190-192, 231-233, 274-278
cedar-policy/src/ffi/format.rs 🔴 0/3 0.00% 44-46
cedar-policy/src/ffi/is_authorized.rs 🔴 0/25 0.00% 66-68, 126-131, 163-168, 202-206, 681-685
cedar-policy/src/ffi/validate.rs 🔴 0/3 0.00% 47-49
cedar-wasm/src/utils.rs 🔴 0/12 0.00% 35-40, 64-69

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 88.22%

Status: PASSED ✅

Details
Package Status Covered Coverage Base Coverage
cedar-language-server 🟢 4722/5102 92.55% --
cedar-policy 🟢 4849/6027 80.45% --
cedar-policy-cli 🟡 1294/1669 77.53% --
cedar-policy-core 🟢 24673/27843 88.61% --
cedar-policy-formatter 🟢 914/1088 84.01% --
cedar-policy-symcc 🟢 6930/7405 93.59% --
cedar-wasm 🔴 0/40 0.00% --

@github-actions

Copy link
Copy Markdown

Coverage Report

Head Commit: f2929e7d8af77959a15436f1c44e99c61d3e00ae

Base Commit: b8fc36d497501d97b8be977d16ffed94b76a89b5

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 0.00%

Status: FAILED ❌

Details
File Status Covered Coverage Missed Lines
cedar-policy/src/ffi/check_parse.rs 🔴 0/18 0.00% 43-47, 89-91, 196-200, 251-255
cedar-policy/src/ffi/convert.rs 🔴 0/28 0.00% 42-46, 88-90, 112-114, 136-138, 163-165, 190-192, 231-233, 274-278
cedar-policy/src/ffi/format.rs 🔴 0/3 0.00% 44-46
cedar-policy/src/ffi/is_authorized.rs 🔴 0/25 0.00% 66-68, 126-131, 163-168, 202-206, 681-685
cedar-policy/src/ffi/validate.rs 🔴 0/3 0.00% 47-49
cedar-wasm/src/utils.rs 🔴 0/12 0.00% 35-40, 64-69

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 88.22%

Status: PASSED ✅

Details
Package Status Covered Coverage Base Coverage
cedar-language-server 🟢 4722/5102 92.55% --
cedar-policy 🟢 4849/6027 80.45% --
cedar-policy-cli 🟡 1294/1669 77.53% --
cedar-policy-core 🟢 24673/27843 88.61% --
cedar-policy-formatter 🟢 914/1088 84.01% --
cedar-policy-symcc 🟢 6930/7405 93.59% --
cedar-wasm 🔴 0/40 0.00% --

Signed-off-by: Katherine Hough <kmhough@amazon.com>
@github-actions

Copy link
Copy Markdown

Coverage Report

Head Commit: ce93459214f555bcd0bc2ff860935de0d9ee3695

Base Commit: b8fc36d497501d97b8be977d16ffed94b76a89b5

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 0.00%

Status: FAILED ❌

Details
File Status Covered Coverage Missed Lines
cedar-policy/src/ffi/check_parse.rs 🔴 0/18 0.00% 43-47, 89-91, 196-200, 251-255
cedar-policy/src/ffi/convert.rs 🔴 0/28 0.00% 42-46, 88-90, 112-114, 136-138, 163-165, 190-192, 231-233, 274-278
cedar-policy/src/ffi/format.rs 🔴 0/3 0.00% 44-46
cedar-policy/src/ffi/is_authorized.rs 🔴 0/25 0.00% 66-68, 126-131, 163-168, 202-206, 681-685
cedar-policy/src/ffi/validate.rs 🔴 0/3 0.00% 47-49
cedar-wasm/src/utils.rs 🔴 0/12 0.00% 35-40, 64-69

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 88.23%

Status: PASSED ✅

Details
Package Status Covered Coverage Base Coverage
cedar-language-server 🟢 4722/5102 92.55% --
cedar-policy 🟢 4849/6027 80.45% --
cedar-policy-cli 🟡 1294/1669 77.53% --
cedar-policy-core 🟢 24667/27832 88.63% --
cedar-policy-formatter 🟢 914/1088 84.01% --
cedar-policy-symcc 🟢 6930/7405 93.59% --
cedar-wasm 🔴 0/40 0.00% --

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update to tsify 0.5.5 or higher

1 participant