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

Add methods for unchecked casting of Any objects. #14217

Closed

Conversation

chris-morgan
Copy link
Member

This is definitely unsafe behaviour, but for certain cases it can be
useful, such as when boxing a value and immediately wanting a reference
to the value as the appropriate type, or when one has already used is
and is thus certain of the type and so wishes to skip the redundant
check.

It'd be handy if we could then implement the checked ones (as_ref,
as_mut_ref and move) on the traits as default methods, but no can do
there, because of the is(self) method calling: an implementation for
the trait objects it will autoreborrow, but in a default implementation
it will consume, so we can't do it that way without restructuring at
least is to take &self, which may have other issues (?), and making
AnyMutRefExt and AnyOwnExt extend AnyRefExt.

This is definitely unsafe behaviour, but for certain cases it can be
useful, such as when boxing a value and immediately wanting a reference
to the value as the appropriate type, or when one has already used `is`
and is thus certain of the type and so wishes to skip the redundant
check.

It'd be handy if we could then implement the checked ones (`as_ref`,
`as_mut_ref` and `move`) on the traits as default methods, but no can do
there, because of the `is(self)` method calling: an implementation for
the trait objects it will autoreborrow, but in a default implementation
it will consume, so we can't do it that way without restructuring at
least `is` to take `&self`, which may have other issues (?), and making
`AnyMutRefExt` and `AnyOwnExt` extend `AnyRefExt`.
@chris-morgan
Copy link
Member Author

(This is an unsolicited contribution; I expect it to be looked on with deep suspicion and will not be surprised if it is rejected.)

As a demonstration of where I want this stuff, my header representation scheme in Teepee needs this for optimal efficiency.

In src/httpcommon/headers/mod.rs I define Header as Any-compatible. Of course, I then need to go implementing all the Any*Ext traits, because while Header extends Any, &Header does not extend &Any, &c. This has the effect of diminishing the value of these unchecked methods to me as I still need to actually implement them, so my maintenance burden is not really changed one way or the other by this going upstream. This, by the way, is a pity. Still, I think there’s value in having them, and it makes the feature that much more official.

The usage of the unchecked methods is in src/httpcommon/headers/internals.rs, lines 125-126 and 155-156: box a value of a known type and immediately take a reference to it; no point in paying the is check twice there. Once it’s being mingled with the Some part, I don’t think you could just add a method which boxed-and-returned-a-reference, so providing the unchecked variety is the easier way to go.

Line 135 also demonstrates another case for it, though there it is just a tad bit of convenience in matching which could be eradicated in favour of using the safe interface, at just the cost of slightly uglier code. (I should probably change it anyway.)

@chris-morgan
Copy link
Member Author

I am curious about the whole implement-the-safe-methods-on-the-trait thing; would making is take &self be feasible? (As it is, it proceeds to call get_type_id which takes &self.) If it was, we could use default methods, and so the implementation burden for people wanting to use types that extend Any with methods of their own, like my Header, would be back to being smaller.

@chris-morgan
Copy link
Member Author

Before putting the effort into rebasing this against upstream changes—is this addition acceptable, O ye members of the core team?

@alexcrichton
Copy link
Member

Closing due to inactivity. I'm not sure how far we want to go down this *_unchecked path. There is not much precedent (although there is some) in today's libraries. For now I'm tempted to say that we can get by without these functions, but if the need definitely does arise, we can definitely reconsider this.

@chris-morgan
Copy link
Member Author

The inactivity is simply due to me waiting for a decision from the team, so I think that citing that as the reason for closing it simultaneously with giving the opinion originally solicited is not fair.

I would say that there is precedent for such things; for example, a string is UTF-8-checked, but there is a method to skip that checking. I demonstrated my use case—though in it for other technical reasons I would not be using the actual implementation of the unchecked methods, rather having to reimplement them for another trait definition, which certainly weakens my position.

@alexcrichton
Copy link
Member

You must understand that we are all quite busy people, and sometimes PRs like this fall through the cracks. It is not solely our responsibility to ensure that every last pull request receives a detailed review. It is often up to the contributor to actively seek out review if it appears to be inactive for quite some time.

You're always free to reopen this, I've stated my opinion, but mine is far from the only one.

lnicola pushed a commit to lnicola/rust that referenced this pull request Mar 13, 2023
fix: Watch both stdout and stderr in flycheck

Fixes rust-lang#14217

This isn't great because it un-mixes the messages from the two streams, but maybe it's not such a big problem?
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.

None yet

2 participants