Skip to content

Change From<&str> to From<&'static str> for Name#24544

Open
bytemuck wants to merge 1 commit into
bevyengine:mainfrom
bytemuck:24465-heap-allocations
Open

Change From<&str> to From<&'static str> for Name#24544
bytemuck wants to merge 1 commit into
bevyengine:mainfrom
bytemuck:24465-heap-allocations

Conversation

@bytemuck
Copy link
Copy Markdown
Contributor

@bytemuck bytemuck commented Jun 6, 2026

Objective

Solution

  • Changed impl From<&str> for Name to impl From<&'static str> for Name

Testing

  • The only place it is (the From<&str> implementation) used is on this test, line 1752, which still passes:
    fn test_animation_target_id() {
    let paths = &[
    vec![],
    vec![""],
    vec!["", ""],
    vec!["a"],
    vec!["a", "a"],
    vec!["a", "b"],
    vec!["b", "a"],
    vec!["aa"],
    vec!["ab"],
    vec!["ba"],
    vec!["abc"],
    vec!["a", "b", "c"],
    vec!["ab", "c"],
    vec!["a", "bc"],
    ];
    // Test that different paths map to a different `AnimationTargetId`.
    for (li, lp) in paths.iter().enumerate() {
    for rp in paths.iter().skip(li + 1) {
    let lt = AnimationTargetId::from_iter(lp.iter());
    let rt = AnimationTargetId::from_iter(rp.iter());
    assert!(lt != rt, "{:?} and {:?} collided. {:?} ", lp, rp, lt);
    }
    }
    // Test that `from_iter` is equivalent to `from_names`.
    for str_path in paths {
    let name_path = str_path.iter().map(|&s| Name::from(s)).collect::<Vec<_>>();
    assert_eq!(
    AnimationTargetId::from_iter(str_path),
    AnimationTargetId::from_names(name_path.iter()),
    "{:?} {:?}",
    str_path,
    name_path
    );
    }
    }

@bytemuck bytemuck marked this pull request as ready for review June 6, 2026 16:02
Copy link
Copy Markdown

@mardzie mardzie left a comment

Choose a reason for hiding this comment

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

Looks great to me.

@kfc35 kfc35 added C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it A-Utils Utility functions and types D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Jun 6, 2026
@SkiFire13
Copy link
Copy Markdown
Contributor

Note that this is a breaking change, so it should be documented in the migration guide.

@bytemuck
Copy link
Copy Markdown
Contributor Author

bytemuck commented Jun 6, 2026

Note that this is a breaking change, so it should be documented in the migration guide.

Is worth to have both impl From<&str> for Name and impl From<&'static str> for Name ?

@kfc35 kfc35 added M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jun 6, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 6, 2026

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

@SkiFire13
Copy link
Copy Markdown
Contributor

Is worth to have both impl From<&str> for Name and impl From<&'static str> for Name ?

You cannot have both, they conflict with each other. Doing something different when a lifetime is 'static is exactly where specialization breaks, so even with that it would not be possible to do this.

@bytemuck
Copy link
Copy Markdown
Contributor Author

bytemuck commented Jun 6, 2026

Is worth to have both impl From<&str> for Name and impl From<&'static str> for Name ?

You cannot have both, they conflict with each other. Doing something different when a lifetime is 'static is exactly where specialization breaks, so even with that it would not be possible to do this.

Thank you for the explanation, I'll do the migration guide since this is a breaking change.

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

Labels

A-Utils Utility functions and types C-Performance A change motivated by improving speed, memory usage or compile times D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Name::from<&str> always heap-allocates which is a footgun

4 participants