delegation: simplify matches on FnKind, minor refactorings

[aerooneqq]

This PR refactors matches on FnKind making them smaller and more concise, next in all matches except fn_kinds function we no longer panic on delegation to inherent impls. And some minor renamings/refactorings. First step for #160505.

Part of #118212.
r? @petrochenkov

[petrochenkov]

I'm generally skeptical about introduction of the extension trait.
It would be one thing if it was actually used as a trait (e.g. in bounds), or the methods were somehow "inherent" to TyCtxt.
But doing it just to use the method call syntax? I don't know.

Removing unnecessary lifetime parameters (#160853 (comment)) will make things nicer if the current setup without the extension trait is kept.

[petrochenkov]

The logic behind the fn kind matching refactoring is also not very clear to me.
Marking the arms as unreachable is good, merging arms using the same logic like in get_delegation_parent_args_count_without_self is good, making the matches less exhaustive - maybe not so good for readability.

[aerooneqq]

Reverted creation of extension to TyCtxt, But doing it just to use the method call syntax? - yes, just esthetical thought that it would look better when match-like functions will be grouped together and calling them as method calls, not that important for now.

making the matches less exhaustive - maybe not so good for readability

I think exhaustive matching does not allow to see the general strategy of handling different FnKinds. Wildcard patterns allow to clearly see general case while all other arms are small and represent edge cases, where one of them is for unreachable!().

In get_delegation_parent_args_count_without_self it is better because the code became more compact and we have an explicit general case handling which uses tcx.generics_of, while in the original version tcx.generics_of was executed all the time at the beginning of the method. Plus the size of patterns in each arm reduced significantly which makes code more understandable.

In get_parent_and_inheritance_kind it is now evident what is the boolean parameter of WithParent variant means plus it is more semantically correct as in original version we returned true for (FnKind::Free, FnKind::Free). + arms' patterns size reduction.

In get_delegation_self_ty we clearly highlight that we create concrete type only when delegation is inside impl (trait or inherent), in all other cases we delegate core logic of creation of self_ty to create_self_param_position_kind, it is explicitly shown with wildcard pattern, while in the original exhaustive match it is not that clear.

So I like new matches more than old ones (wildcard patterns reduce thinking overhead which is needed when first-time looking at exhaustive pattern with 4 tuples of FnKind and trying to understand what all those cases have in common and where to add potential new one), and overall code size reduction is good I think.

@rustbot ready

[petrochenkov]

r=me after squashing commits.
@rustbot author
@bors delegate+

[rust-bors]

✌️ @aerooneqq, you can now approve this pull request!

If @petrochenkov told you to "r=me" after making some further change, then please make that change and post @bors r=petrochenkov.

View changes since this delegation.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[aerooneqq]

@bors squash

[rust-bors]

🔨 5 commits were squashed into d6e4a1d.

[aerooneqq]

@bors r=petrochenkov

[rust-bors]

📌 Commit d6e4a1d has been approved by petrochenkov

It is now in the queue for this repository.

[aerooneqq]

This PR refactors matches on FnKind making them smaller and more concise, next in all matches except fn_kinds function we no longer panic on delegation to inherent impls. And some minor renamings/refactorings. First step for #160505.

Part of #118212.
r? @petrochenkov

[petrochenkov]

I'm generally skeptical about introduction of the extension trait.
It would be one thing if it was actually used as a trait (e.g. in bounds), or the methods were somehow "inherent" to TyCtxt.
But doing it just to use the method call syntax? I don't know.

Removing unnecessary lifetime parameters (#160853 (comment)) will make things nicer if the current setup without the extension trait is kept.

[petrochenkov]

The logic behind the fn kind matching refactoring is also not very clear to me.
Marking the arms as unreachable is good, merging arms using the same logic like in get_delegation_parent_args_count_without_self is good, making the matches less exhaustive - maybe not so good for readability.

[aerooneqq]

Reverted creation of extension to TyCtxt, But doing it just to use the method call syntax? - yes, just esthetical thought that it would look better when match-like functions will be grouped together and calling them as method calls, not that important for now.

making the matches less exhaustive - maybe not so good for readability

I think exhaustive matching does not allow to see the general strategy of handling different FnKinds. Wildcard patterns allow to clearly see general case while all other arms are small and represent edge cases, where one of them is for unreachable!().

In get_delegation_parent_args_count_without_self it is better because the code became more compact and we have an explicit general case handling which uses tcx.generics_of, while in the original version tcx.generics_of was executed all the time at the beginning of the method. Plus the size of patterns in each arm reduced significantly which makes code more understandable.

In get_parent_and_inheritance_kind it is now evident what is the boolean parameter of WithParent variant means plus it is more semantically correct as in original version we returned true for (FnKind::Free, FnKind::Free). + arms' patterns size reduction.

In get_delegation_self_ty we clearly highlight that we create concrete type only when delegation is inside impl (trait or inherent), in all other cases we delegate core logic of creation of self_ty to create_self_param_position_kind, it is explicitly shown with wildcard pattern, while in the original exhaustive match it is not that clear.

So I like new matches more than old ones (wildcard patterns reduce thinking overhead which is needed when first-time looking at exhaustive pattern with 4 tuples of FnKind and trying to understand what all those cases have in common and where to add potential new one), and overall code size reduction is good I think.

@rustbot ready

[petrochenkov]

r=me after squashing commits.
@rustbot author
@bors delegate+

[rust-bors]

✌️ @aerooneqq, you can now approve this pull request!

If @petrochenkov told you to "r=me" after making some further change, then please make that change and post @bors r=petrochenkov.

View changes since this delegation.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[aerooneqq]

@bors squash

[rust-bors]

🔨 5 commits were squashed into d6e4a1d.

[aerooneqq]

@bors r=petrochenkov

[rust-bors]

📌 Commit d6e4a1d has been approved by petrochenkov

It is now in the queue for this repository.

[aerooneqq]

This PR refactors matches on FnKind making them smaller and more concise, next in all matches except fn_kinds function we no longer panic on delegation to inherent impls. And some minor renamings/refactorings. First step for #160505.

Part of #118212.
r? @petrochenkov

[petrochenkov]

I'm generally skeptical about introduction of the extension trait.
It would be one thing if it was actually used as a trait (e.g. in bounds), or the methods were somehow "inherent" to TyCtxt.
But doing it just to use the method call syntax? I don't know.

Removing unnecessary lifetime parameters (#160853 (comment)) will make things nicer if the current setup without the extension trait is kept.

[petrochenkov]

The logic behind the fn kind matching refactoring is also not very clear to me.
Marking the arms as unreachable is good, merging arms using the same logic like in get_delegation_parent_args_count_without_self is good, making the matches less exhaustive - maybe not so good for readability.

[aerooneqq]

Reverted creation of extension to TyCtxt, But doing it just to use the method call syntax? - yes, just esthetical thought that it would look better when match-like functions will be grouped together and calling them as method calls, not that important for now.

making the matches less exhaustive - maybe not so good for readability

I think exhaustive matching does not allow to see the general strategy of handling different FnKinds. Wildcard patterns allow to clearly see general case while all other arms are small and represent edge cases, where one of them is for unreachable!().

In get_delegation_parent_args_count_without_self it is better because the code became more compact and we have an explicit general case handling which uses tcx.generics_of, while in the original version tcx.generics_of was executed all the time at the beginning of the method. Plus the size of patterns in each arm reduced significantly which makes code more understandable.

In get_parent_and_inheritance_kind it is now evident what is the boolean parameter of WithParent variant means plus it is more semantically correct as in original version we returned true for (FnKind::Free, FnKind::Free). + arms' patterns size reduction.

In get_delegation_self_ty we clearly highlight that we create concrete type only when delegation is inside impl (trait or inherent), in all other cases we delegate core logic of creation of self_ty to create_self_param_position_kind, it is explicitly shown with wildcard pattern, while in the original exhaustive match it is not that clear.

So I like new matches more than old ones (wildcard patterns reduce thinking overhead which is needed when first-time looking at exhaustive pattern with 4 tuples of FnKind and trying to understand what all those cases have in common and where to add potential new one), and overall code size reduction is good I think.

@rustbot ready

[petrochenkov]

r=me after squashing commits.
@rustbot author
@bors delegate+

[rust-bors]

✌️ @aerooneqq, you can now approve this pull request!

If @petrochenkov told you to "r=me" after making some further change, then please make that change and post @bors r=petrochenkov.

View changes since this delegation.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[aerooneqq]

@bors squash

[rust-bors]

🔨 5 commits were squashed into d6e4a1d.

[aerooneqq]

@bors r=petrochenkov

[rust-bors]

📌 Commit d6e4a1d has been approved by petrochenkov

It is now in the queue for this repository.

[aerooneqq]

This PR refactors matches on FnKind making them smaller and more concise, next in all matches except fn_kinds function we no longer panic on delegation to inherent impls. And some minor renamings/refactorings. First step for #160505.

Part of #118212.
r? @petrochenkov

[petrochenkov]

I'm generally skeptical about introduction of the extension trait.
It would be one thing if it was actually used as a trait (e.g. in bounds), or the methods were somehow "inherent" to TyCtxt.
But doing it just to use the method call syntax? I don't know.

Removing unnecessary lifetime parameters (#160853 (comment)) will make things nicer if the current setup without the extension trait is kept.

[petrochenkov]

The logic behind the fn kind matching refactoring is also not very clear to me.
Marking the arms as unreachable is good, merging arms using the same logic like in get_delegation_parent_args_count_without_self is good, making the matches less exhaustive - maybe not so good for readability.

[aerooneqq]

Reverted creation of extension to TyCtxt, But doing it just to use the method call syntax? - yes, just esthetical thought that it would look better when match-like functions will be grouped together and calling them as method calls, not that important for now.

making the matches less exhaustive - maybe not so good for readability

I think exhaustive matching does not allow to see the general strategy of handling different FnKinds. Wildcard patterns allow to clearly see general case while all other arms are small and represent edge cases, where one of them is for unreachable!().

In get_delegation_parent_args_count_without_self it is better because the code became more compact and we have an explicit general case handling which uses tcx.generics_of, while in the original version tcx.generics_of was executed all the time at the beginning of the method. Plus the size of patterns in each arm reduced significantly which makes code more understandable.

In get_parent_and_inheritance_kind it is now evident what is the boolean parameter of WithParent variant means plus it is more semantically correct as in original version we returned true for (FnKind::Free, FnKind::Free). + arms' patterns size reduction.

In get_delegation_self_ty we clearly highlight that we create concrete type only when delegation is inside impl (trait or inherent), in all other cases we delegate core logic of creation of self_ty to create_self_param_position_kind, it is explicitly shown with wildcard pattern, while in the original exhaustive match it is not that clear.

So I like new matches more than old ones (wildcard patterns reduce thinking overhead which is needed when first-time looking at exhaustive pattern with 4 tuples of FnKind and trying to understand what all those cases have in common and where to add potential new one), and overall code size reduction is good I think.

@rustbot ready

[petrochenkov]

r=me after squashing commits.
@rustbot author
@bors delegate+

[rust-bors]

✌️ @aerooneqq, you can now approve this pull request!

If @petrochenkov told you to "r=me" after making some further change, then please make that change and post @bors r=petrochenkov.

View changes since this delegation.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[aerooneqq]

@bors squash

[rust-bors]

🔨 5 commits were squashed into d6e4a1d.

[aerooneqq]

@bors r=petrochenkov

[rust-bors]

📌 Commit d6e4a1d has been approved by petrochenkov

It is now in the queue for this repository.

[aerooneqq]

This PR refactors matches on FnKind making them smaller and more concise, next in all matches except fn_kinds function we no longer panic on delegation to inherent impls. And some minor renamings/refactorings. First step for #160505.

Part of #118212.
r? @petrochenkov

[petrochenkov]

I'm generally skeptical about introduction of the extension trait.
It would be one thing if it was actually used as a trait (e.g. in bounds), or the methods were somehow "inherent" to TyCtxt.
But doing it just to use the method call syntax? I don't know.

Removing unnecessary lifetime parameters (#160853 (comment)) will make things nicer if the current setup without the extension trait is kept.

[petrochenkov]

The logic behind the fn kind matching refactoring is also not very clear to me.
Marking the arms as unreachable is good, merging arms using the same logic like in get_delegation_parent_args_count_without_self is good, making the matches less exhaustive - maybe not so good for readability.

[aerooneqq]

Reverted creation of extension to TyCtxt, But doing it just to use the method call syntax? - yes, just esthetical thought that it would look better when match-like functions will be grouped together and calling them as method calls, not that important for now.

making the matches less exhaustive - maybe not so good for readability

I think exhaustive matching does not allow to see the general strategy of handling different FnKinds. Wildcard patterns allow to clearly see general case while all other arms are small and represent edge cases, where one of them is for unreachable!().

In get_delegation_parent_args_count_without_self it is better because the code became more compact and we have an explicit general case handling which uses tcx.generics_of, while in the original version tcx.generics_of was executed all the time at the beginning of the method. Plus the size of patterns in each arm reduced significantly which makes code more understandable.

In get_parent_and_inheritance_kind it is now evident what is the boolean parameter of WithParent variant means plus it is more semantically correct as in original version we returned true for (FnKind::Free, FnKind::Free). + arms' patterns size reduction.

In get_delegation_self_ty we clearly highlight that we create concrete type only when delegation is inside impl (trait or inherent), in all other cases we delegate core logic of creation of self_ty to create_self_param_position_kind, it is explicitly shown with wildcard pattern, while in the original exhaustive match it is not that clear.

So I like new matches more than old ones (wildcard patterns reduce thinking overhead which is needed when first-time looking at exhaustive pattern with 4 tuples of FnKind and trying to understand what all those cases have in common and where to add potential new one), and overall code size reduction is good I think.

@rustbot ready

[petrochenkov]

r=me after squashing commits.
@rustbot author
@bors delegate+

[rust-bors]

✌️ @aerooneqq, you can now approve this pull request!

If @petrochenkov told you to "r=me" after making some further change, then please make that change and post @bors r=petrochenkov.

View changes since this delegation.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[aerooneqq]

@bors squash

[rust-bors]

🔨 5 commits were squashed into d6e4a1d.

[aerooneqq]

@bors r=petrochenkov

[rust-bors]

📌 Commit d6e4a1d has been approved by petrochenkov

It is now in the queue for this repository.

[aerooneqq]

This PR refactors matches on FnKind making them smaller and more concise, next in all matches except fn_kinds function we no longer panic on delegation to inherent impls. And some minor renamings/refactorings. First step for #160505.

Part of #118212.
r? @petrochenkov

[petrochenkov]

I'm generally skeptical about introduction of the extension trait.
It would be one thing if it was actually used as a trait (e.g. in bounds), or the methods were somehow "inherent" to TyCtxt.
But doing it just to use the method call syntax? I don't know.

Removing unnecessary lifetime parameters (#160853 (comment)) will make things nicer if the current setup without the extension trait is kept.

[petrochenkov]

The logic behind the fn kind matching refactoring is also not very clear to me.
Marking the arms as unreachable is good, merging arms using the same logic like in get_delegation_parent_args_count_without_self is good, making the matches less exhaustive - maybe not so good for readability.

[aerooneqq]

Reverted creation of extension to TyCtxt, But doing it just to use the method call syntax? - yes, just esthetical thought that it would look better when match-like functions will be grouped together and calling them as method calls, not that important for now.

making the matches less exhaustive - maybe not so good for readability

I think exhaustive matching does not allow to see the general strategy of handling different FnKinds. Wildcard patterns allow to clearly see general case while all other arms are small and represent edge cases, where one of them is for unreachable!().

In get_delegation_parent_args_count_without_self it is better because the code became more compact and we have an explicit general case handling which uses tcx.generics_of, while in the original version tcx.generics_of was executed all the time at the beginning of the method. Plus the size of patterns in each arm reduced significantly which makes code more understandable.

In get_parent_and_inheritance_kind it is now evident what is the boolean parameter of WithParent variant means plus it is more semantically correct as in original version we returned true for (FnKind::Free, FnKind::Free). + arms' patterns size reduction.

In get_delegation_self_ty we clearly highlight that we create concrete type only when delegation is inside impl (trait or inherent), in all other cases we delegate core logic of creation of self_ty to create_self_param_position_kind, it is explicitly shown with wildcard pattern, while in the original exhaustive match it is not that clear.

So I like new matches more than old ones (wildcard patterns reduce thinking overhead which is needed when first-time looking at exhaustive pattern with 4 tuples of FnKind and trying to understand what all those cases have in common and where to add potential new one), and overall code size reduction is good I think.

@rustbot ready

[petrochenkov]

r=me after squashing commits.
@rustbot author
@bors delegate+

[rust-bors]

✌️ @aerooneqq, you can now approve this pull request!

If @petrochenkov told you to "r=me" after making some further change, then please make that change and post @bors r=petrochenkov.

View changes since this delegation.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[aerooneqq]

@bors squash

[rust-bors]

🔨 5 commits were squashed into d6e4a1d.

[aerooneqq]

@bors r=petrochenkov

[rust-bors]

📌 Commit d6e4a1d has been approved by petrochenkov

It is now in the queue for this repository.

[aerooneqq]

This PR refactors matches on FnKind making them smaller and more concise, next in all matches except fn_kinds function we no longer panic on delegation to inherent impls. And some minor renamings/refactorings. First step for #160505.

Part of #118212.
r? @petrochenkov

[petrochenkov]

I'm generally skeptical about introduction of the extension trait.
It would be one thing if it was actually used as a trait (e.g. in bounds), or the methods were somehow "inherent" to TyCtxt.
But doing it just to use the method call syntax? I don't know.

Removing unnecessary lifetime parameters (#160853 (comment)) will make things nicer if the current setup without the extension trait is kept.

[petrochenkov]

The logic behind the fn kind matching refactoring is also not very clear to me.
Marking the arms as unreachable is good, merging arms using the same logic like in get_delegation_parent_args_count_without_self is good, making the matches less exhaustive - maybe not so good for readability.

[aerooneqq]

Reverted creation of extension to TyCtxt, But doing it just to use the method call syntax? - yes, just esthetical thought that it would look better when match-like functions will be grouped together and calling them as method calls, not that important for now.

making the matches less exhaustive - maybe not so good for readability

I think exhaustive matching does not allow to see the general strategy of handling different FnKinds. Wildcard patterns allow to clearly see general case while all other arms are small and represent edge cases, where one of them is for unreachable!().

In get_delegation_parent_args_count_without_self it is better because the code became more compact and we have an explicit general case handling which uses tcx.generics_of, while in the original version tcx.generics_of was executed all the time at the beginning of the method. Plus the size of patterns in each arm reduced significantly which makes code more understandable.

In get_parent_and_inheritance_kind it is now evident what is the boolean parameter of WithParent variant means plus it is more semantically correct as in original version we returned true for (FnKind::Free, FnKind::Free). + arms' patterns size reduction.

In get_delegation_self_ty we clearly highlight that we create concrete type only when delegation is inside impl (trait or inherent), in all other cases we delegate core logic of creation of self_ty to create_self_param_position_kind, it is explicitly shown with wildcard pattern, while in the original exhaustive match it is not that clear.

So I like new matches more than old ones (wildcard patterns reduce thinking overhead which is needed when first-time looking at exhaustive pattern with 4 tuples of FnKind and trying to understand what all those cases have in common and where to add potential new one), and overall code size reduction is good I think.

@rustbot ready

[petrochenkov]

r=me after squashing commits.
@rustbot author
@bors delegate+

[rust-bors]

✌️ @aerooneqq, you can now approve this pull request!

If @petrochenkov told you to "r=me" after making some further change, then please make that change and post @bors r=petrochenkov.

View changes since this delegation.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[aerooneqq]

@bors squash

[rust-bors]

🔨 5 commits were squashed into d6e4a1d.

[aerooneqq]

@bors r=petrochenkov

[rust-bors]

📌 Commit d6e4a1d has been approved by petrochenkov

It is now in the queue for this repository.

[aerooneqq]

This PR refactors matches on FnKind making them smaller and more concise, next in all matches except fn_kinds function we no longer panic on delegation to inherent impls. And some minor renamings/refactorings. First step for #160505.

Part of #118212.
r? @petrochenkov

[petrochenkov]

I'm generally skeptical about introduction of the extension trait.
It would be one thing if it was actually used as a trait (e.g. in bounds), or the methods were somehow "inherent" to TyCtxt.
But doing it just to use the method call syntax? I don't know.

Removing unnecessary lifetime parameters (#160853 (comment)) will make things nicer if the current setup without the extension trait is kept.

[petrochenkov]

The logic behind the fn kind matching refactoring is also not very clear to me.
Marking the arms as unreachable is good, merging arms using the same logic like in get_delegation_parent_args_count_without_self is good, making the matches less exhaustive - maybe not so good for readability.

[aerooneqq]

Reverted creation of extension to TyCtxt, But doing it just to use the method call syntax? - yes, just esthetical thought that it would look better when match-like functions will be grouped together and calling them as method calls, not that important for now.

making the matches less exhaustive - maybe not so good for readability

I think exhaustive matching does not allow to see the general strategy of handling different FnKinds. Wildcard patterns allow to clearly see general case while all other arms are small and represent edge cases, where one of them is for unreachable!().

In get_delegation_parent_args_count_without_self it is better because the code became more compact and we have an explicit general case handling which uses tcx.generics_of, while in the original version tcx.generics_of was executed all the time at the beginning of the method. Plus the size of patterns in each arm reduced significantly which makes code more understandable.

In get_parent_and_inheritance_kind it is now evident what is the boolean parameter of WithParent variant means plus it is more semantically correct as in original version we returned true for (FnKind::Free, FnKind::Free). + arms' patterns size reduction.

In get_delegation_self_ty we clearly highlight that we create concrete type only when delegation is inside impl (trait or inherent), in all other cases we delegate core logic of creation of self_ty to create_self_param_position_kind, it is explicitly shown with wildcard pattern, while in the original exhaustive match it is not that clear.

So I like new matches more than old ones (wildcard patterns reduce thinking overhead which is needed when first-time looking at exhaustive pattern with 4 tuples of FnKind and trying to understand what all those cases have in common and where to add potential new one), and overall code size reduction is good I think.

@rustbot ready

[petrochenkov]

r=me after squashing commits.
@rustbot author
@bors delegate+

[rust-bors]

✌️ @aerooneqq, you can now approve this pull request!

If @petrochenkov told you to "r=me" after making some further change, then please make that change and post @bors r=petrochenkov.

View changes since this delegation.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[aerooneqq]

@bors squash

[rust-bors]

🔨 5 commits were squashed into d6e4a1d.

[aerooneqq]

@bors r=petrochenkov

[rust-bors]

📌 Commit d6e4a1d has been approved by petrochenkov

It is now in the queue for this repository.