Skip to content

Conversation

iamsauravsharma
Copy link
Contributor

@iamsauravsharma iamsauravsharma commented Mar 9, 2024

Description of changes

Return static lifetime for ValidationError and ValidationWarning so lifetime will live enough. 3.1.0 is not working with latest nightly version this PR fix that issue by returning static lifetime instead of custom lifetime
Latest nightly error message

error: lifetime may not live long enough
    --> cedar-policy/src/api.rs:1677:9
     |
1676 |     pub fn validation_errors<'b>(&self) -> impl Iterator<Item = &ValidationError<'b>> {
     |                              --  - let's call the lifetime of this reference `'1`
     |                              |
     |                              lifetime `'b` defined here
1677 |         self.validation_errors.iter()
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ method was supposed to return data with lifetime `'1` but it is returning data with lifetime `'b`

error: lifetime may not live long enough
    --> cedar-policy/src/api.rs:1682:9
     |
1681 |     pub fn validation_warnings<'b>(&self) -> impl Iterator<Item = &ValidationWarning<'b>> {
     |                                --  - let's call the lifetime of this reference `'1`
     |                                |
     |                                lifetime `'b` defined here
1682 |         self.validation_warnings.iter()
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ method was supposed to return data with lifetime `'1` but it is returning data with lifetime `'b`

error: lifetime may not live long enough
    --> cedar-policy/src/api.rs:1920:5
     |
1914 |   pub fn confusable_string_checker<'a, 'b>(
     |                                        -- lifetime `'b` defined here
...
1920 | /     cedar_policy_validator::confusable_string_checks(templates.map(|t| &t.ast))
1921 | |         .map(std::convert::Into::into)
     | |______________________________________^ returning this value requires that `'b` must outlive `'static`

error: could not compile `cedar-policy` (lib) due to 3 previous errors

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.

Disclaimer

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@iamsauravsharma
Copy link
Contributor Author

iamsauravsharma commented Mar 10, 2024

I have already created issue at rust-lang/rust#122230 which informs it is expected breakage so this PR fixes issue early for unsoundess. For more information visit issue

@iamsauravsharma iamsauravsharma changed the title refactor: return static lifetime for validation errors and warnings fix: unsoundness issue catched in recent nightly Mar 10, 2024
@iamsauravsharma iamsauravsharma changed the title fix: unsoundness issue catched in recent nightly fix: unsoundness issue caught in recent nightly Mar 10, 2024
Copy link
Contributor

@john-h-kastner-aws john-h-kastner-aws left a comment

Choose a reason for hiding this comment

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

Thanks for catching this.

We originally wrote it how we did because returning a static lifetime caused some dependencies builds to break due to something around lifetime inference, but (at least according to CI) this seems to work.

@iamsauravsharma
Copy link
Contributor Author

iamsauravsharma commented Mar 11, 2024

@john-h-kastner-aws I have removed some unnecessary lifetimes to solve above issue. All lifetimes are not required to create above changed struct. This solve issue by removing lifetime altogether

@john-h-kastner-aws
Copy link
Contributor

I believe that would be a breaking change for any user who references the types with explicit lifetimes. We can take that change on main, but will likely patch 3.1 with your original commit.

@iamsauravsharma
Copy link
Contributor Author

Ya you are correct. I think I will only add original changes to this PR and create new PR for next breaking changes

@iamsauravsharma
Copy link
Contributor Author

iamsauravsharma commented Mar 11, 2024

@john-h-kastner-aws I have reverted to original fix in this PR and created another PR #715 for breaking change

@john-h-kastner-aws john-h-kastner-aws merged commit 97d95c0 into cedar-policy:main Mar 11, 2024
@iamsauravsharma iamsauravsharma deleted the fix-nightly branch March 11, 2024 16:56
khieta pushed a commit that referenced this pull request Mar 11, 2024
Signed-off-by: Saurav Sharma <[email protected]>
Signed-off-by: Kesha Hietala <[email protected]>
john-h-kastner-aws pushed a commit that referenced this pull request Mar 12, 2024
john-h-kastner-aws added a commit that referenced this pull request Mar 12, 2024
Signed-off-by: Saurav Sharma <[email protected]>
Signed-off-by: John Kastner <[email protected]>
Co-authored-by: Saurav Sharma <[email protected]>
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.

4 participants