Skip to content

Fix type inference for slicing in some cases #422

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

Merged
merged 1 commit into from
Mar 17, 2018

Conversation

jturner314
Copy link
Member

In some cases, the compiler does not infer a dimension type for a sliced array. (It says "multiple applicable items in scope" when calling .dot() on the sliced array.) See the test_slice_infer test in this PR.

A partial fix is to implement SliceNextDim for more Range* types. I'm not quite sure why this works, and inference is still broken for integers that aren't part of ranges (the commented-out line in test_slice_infer), but it's an improvement.

It's worth noting that I also tried implementing SliceNextDim for Range*<u8>, Range*<u16>, … (all primitive integer types) instead of Range*<T> where T: PrimInt, but that didn't fix the issue.

I've tried fixing inference for integers that aren't part of ranges, but I haven't had any success. Some of the things I've tried:

  • Implementing SliceNextDim for u8, u16, … (all primitive integer types), but that didn't fix the issue.
  • Changing the D2 generic type parameter in SliceNextDim<D1, D2> to an associated type. I think that's a better design anyway (and I'll create a PR for this later since it's a breaking change), but it didn't fix this issue.
  • Adding another trait SliceNextDimInner<D> and then implementing SliceNextDim for T: SliceNextDimInner<D>, but that didn't fix the issue.

Does anyone have suggestions for fixing inference with integers that aren't part of ranges?

@bluss
Copy link
Member

bluss commented Mar 10, 2018

I'm a bit skeptical of the fix. Why are we even using PrimInt here? Maybe that part is not needed. This doesn't seem ideal either way we do it, but I'd prefer to not use PrimInt, and just implement SliceNextDim for ranges, in a way, looking only at the inputs "structurally".

The fix is to implement `SliceNextDim` for more types. I'm not quite
sure why this works, and inference is still broken for integers that
aren't part of ranges (the commented-out line in `test_slice_infer`),
but it's an improvement.
@jturner314
Copy link
Member Author

Yeah, the PrimInt bound isn't necessary; I added it just to limit how many additional types for which I was implementing SliceNextDim. (I figured that we could always remove the PrimInt bound in the future without it being a breaking change.) It is somewhat simpler to avoid PrimInt entirely. I pushed a new version that uses T without the PrimInt bound.

Do you have any ideas for fixing inference with integer arguments that aren't part of ranges? (See the commented-out line in test_slice_infer in this PR.)

To me, this issue seems like a compiler bug, and we're just trying to find a workaround. Maybe we should try to fix the compiler instead? (I can work out a minimal example of the issue to submit to rust-lang/rust.)

@bluss
Copy link
Member

bluss commented Mar 13, 2018

Thanks, this is great

I haven't looked at the type inference situation for the single index, sorry. I'm not sure it's a compiler bug, but I think experience from Rust 1.0-1.24 is that the compiler does actually get smarter at such things eventually. So there is hope.

@bluss bluss merged commit 12351ab into rust-ndarray:master Mar 17, 2018
@bluss
Copy link
Member

bluss commented Mar 17, 2018

I'm unsure how to "debug" the single index situation, I haven't looked at it any more. We can fix it later. Thank you for this.

jturner314 added a commit to jturner314/ndarray that referenced this pull request Mar 19, 2018
I forgot to remove these annotations before merging rust-ndarray#421. They were
necessary until after rust-ndarray#422 was merged.
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