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

Depraction warning for external(v0) on impls. #4470

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented Nov 24, 2023

Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.


This change is Reviewable

@orizi orizi force-pushed the pr/orizi/map-lowering-diags/aeac24e1 branch from 220b946 to 29c58be Compare November 24, 2023 11:26
@orizi orizi force-pushed the pr/orizi/map-lowering-diags/a0199fa7 branch from e983673 to cc2ddd7 Compare November 24, 2023 11:26
@orizi orizi force-pushed the pr/orizi/map-lowering-diags/a0199fa7 branch from cc2ddd7 to eb1f3a4 Compare November 26, 2023 17:21
@orizi orizi force-pushed the pr/orizi/map-lowering-diags/aeac24e1 branch 2 times, most recently from dc1fe8c to f974cc0 Compare November 27, 2023 07:46
@orizi orizi force-pushed the pr/orizi/map-lowering-diags/a0199fa7 branch from 4025434 to 7834398 Compare November 27, 2023 07:58
@orizi orizi force-pushed the pr/orizi/map-lowering-diags/aeac24e1 branch from f974cc0 to 4e94079 Compare November 27, 2023 07:58
Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 14 of 16 files at r1, all commit messages.
Reviewable status: 14 of 16 files reviewed, 1 unresolved discussion (waiting on @orizi)


crates/cairo-lang-starknet/src/plugin/utils.rs line 218 at r1 (raw file):

    if let Some(deprecated) = deprecated() {
        diagnostics.push(PluginDiagnostic::warning(attr.stable_ptr().untyped(), deprecated));
    }

It'll raise this warning for any attr_name attribute, in our case, also for #[external(any_text_in_here)], which is not deprecated as it haven't been supported. Maybe validate_v0 should return a bool and the warning should be raised according to it.

Code quote:

    if let Some(deprecated) = deprecated() {
        diagnostics.push(PluginDiagnostic::warning(attr.stable_ptr().untyped(), deprecated));
    }

@orizi orizi force-pushed the pr/orizi/map-lowering-diags/a0199fa7 branch from 7834398 to 73cc967 Compare November 27, 2023 08:44
@orizi orizi force-pushed the pr/orizi/map-lowering-diags/aeac24e1 branch from 4e94079 to 027eac7 Compare November 27, 2023 08:44
Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 14 of 16 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware)


crates/cairo-lang-starknet/src/plugin/utils.rs line 218 at r1 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

It'll raise this warning for any attr_name attribute, in our case, also for #[external(any_text_in_here)], which is not deprecated as it haven't been supported. Maybe validate_v0 should return a bool and the warning should be raised according to it.

the external is what is deprecated - so i'm fine with this.

Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 16 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @orizi)

@orizi orizi force-pushed the pr/orizi/map-lowering-diags/a0199fa7 branch from 73cc967 to 776236c Compare November 27, 2023 09:05
@orizi orizi force-pushed the pr/orizi/map-lowering-diags/aeac24e1 branch from 027eac7 to 7754dce Compare November 27, 2023 09:05
@orizi orizi changed the base branch from pr/orizi/map-lowering-diags/a0199fa7 to main November 27, 2023 09:15
@orizi orizi force-pushed the pr/orizi/map-lowering-diags/aeac24e1 branch from 7754dce to b2f1ed9 Compare November 27, 2023 09:15
@orizi orizi enabled auto-merge November 27, 2023 09:16
@orizi orizi added this pull request to the merge queue Nov 27, 2023
Merged via the queue into main with commit eb0d8c7 Nov 27, 2023
@orizi orizi deleted the pr/orizi/map-lowering-diags/aeac24e1 branch November 29, 2023 11:29
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.

2 participants