suggest one-argument enum variant to fix type mismatch when applicable #43178
Conversation
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
|
I don't know what AppVeyor's problem is. |
|
From the example, it looks like this replaces the function suggestions with the enum constructor. Would it be possible to add the enum constructor as an additional item in the list of suggestions? |
aidanhs
added
the
S-waiting-on-author
The behavior is intentional; discussion on the original issue looked unfavorably on the existing suggestions in this situation (at least in the case of Intuitively, I'd guess that situations that trigger this but where the probe-for-return-type method suggestions would be preferrable are kind of rare? (If you have an |
Most notably, this will suggest `Some(x)` when the expected type was an Option<T> but we got an x: T. Resolves rust-lang#42764.
zackmdavis
force-pushed
the
some_suggestion
branch
from
49bcb16
to
eac7410
Compare
|
(rebased on master and forced-pushed in the hopes of removing the spurious "AppVeyor was unable to build" failure indicator) |
There is an issue opened around what the right way to filter and/or order the suggestions. #42929 . It seems preferable to allow generating the additional suggestion of the enum constructor while we're still in the process of deciding how to order/filter suggestions. |
We want the suggested replacement (which IDE tooling and such might offer to automatically swap in) to, like, actually be correct: suggesting `MyVariant(x)` when the actual fix is `MyEnum::MyVariant(x)` might be better than nothing, but Rust is supposed to be the future of computing: we're better than better than nothing. As an exceptional case, we excise the prelude path, preferring to suggest `Some` or `Ok` rather than `std::prelude::v1::Some` and `std::prelude::v2::Ok`. (It's not worth the effort to future-proof against hypothetical preludes v2, v3, &c.: we trust our successors to grep—excuse me, ripgrep—for that.) Also, don't make this preëmpt the existing probe-for-return-type suggestions, despite their being looked unfavorably upon, at least in this situation (rust-lang#42764 (comment)): Cody Schafer pointed out that that's a separate issue (rust-lang#43178 (comment)). This is in the matter of rust-lang#42764.
|
@jmesmon OK (deleted
Also 80c603f, made the suggestion use the path to the enum rather than the bare name (which might not be in scope). |
|
r? @eddyb |
|
@bors r+ |
|
📌 Commit 80c603f has been approved by |
|
⌛ Testing commit 80c603f with merge b59d66de6045f11bb7663719c019ea0289be94b2... |
|
💔 Test failed - status-appveyor |
|
@bors retry -- intermittent network (service) issue |
|
⌛ Testing commit 80c603f with merge f07dd9ae3be22de126d283ce2e0ca99d68a80cc9... |
|
💔 Test failed - status-appveyor |
|
Unable to download |
|
@kennytm this issue affects all PRs right now. Not sure what the cause of the issue is, but probably either some revocation server is actually down, or there is a networking issue with appveyor that is specific to the revocation servers (generally there seems to be network). I guess we just wait a bit and hope the issue gets fixed by itself, but smashing the retry button won't be very useful.... cc @oli-obk |
|
lets try whether it persists... @bors retry |
|
⌛ Testing commit 80c603f with merge 356d1a7212304f96f686419631256efa4eaea7bd... |
|
💔 Test failed - status-travis |
|
@bors retry -- sscache network issue and not on appveyor |
|
⌛ Testing commit 80c603f with merge a7dffd14a21a338a89b87751a1aadfff1cb52094... |
|
💔 Test failed - status-travis |
|
@bors: retry
…On Wed, Jul 19, 2017 at 6:48 AM, bors ***@***.***> wrote:
💔 Test failed - status-travis
<https://travis-ci.org/rust-lang/rust/builds/255225836?utm_source=github_status&utm_medium=notification>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#43178 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AaPN0IcelMNvUJQ22p40Gif7hOtT81hIks5sPe0UgaJpZM4OVGPD>
.
--
You received this message because you are subscribed to the Google Groups
"rust-ops" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to ***@***.***
To post to this group, send email to ***@***.***
To view this discussion on the web visit https://groups.google.com/d/
msgid/rust-ops/rust-lang/rust/pull/43178/c316360577%40github.com
<https://groups.google.com/d/msgid/rust-ops/rust-lang/rust/pull/43178/c316360577%40github.com?utm_medium=email&utm_source=footer>
.
For more options, visit https://groups.google.com/d/optout.
|
|
@nagbot-rs: 🔑 Insufficient privileges: and not in try users |
|
@bors: retry |
suggest one-argument enum variant to fix type mismatch when applicable Following @est31's [suggestion](#42764 (comment)).  Resolves #42764.
|
☀️ Test successful - status-appveyor, status-travis |
Felix S. Klock II pointed out that this suggestion (introduced in pull-request rust-lang#43178 / eac7410) was being issued for one-field-struct expected types (in which case it is misleading and outright wrong), even though it was only intended for one-field enum-variants (most notably, `Some`). Particularly tender-hearted code-historians may be inclined to show mercy towards the author of rust-lang#43178 on the grounds that it's somewhat confusing that struct field definitions are given in a type called `ty::VariantDef`. Add a conditional to adhere to the original intent. (It would be possible to generalize to structs, but not obviously net desirable.) This adds a level of indentation, so the diff here is going to be easier to read in ignore-whitespace mode (`-w`). Resolves rust-lang#55250.
Felix S. Klock II pointed out that this suggestion (introduced in pull-request rust-lang#43178 / eac7410) was being issued for one-field-struct expected types (in which case it is misleading and outright wrong), even though it was only intended for one-field enum-variants (most notably, `Some`). Particularly tender-hearted code-historians may be inclined to show mercy towards the author of rust-lang#43178 on the grounds that it's somewhat confusing that struct field definitions are given in a type called `ty::VariantDef`. Add a conditional to adhere to the original intent. (It would be possible to generalize to structs, but not obviously net desirable.) This adds a level of indentation, so the diff here is going to be easier to read in ignore-whitespace mode (`-w`). Resolves rust-lang#55250.
only issue "variant of the expected type" suggestion for enums This suggestion (introduced in pull-request rust-lang#43178 / eac7410) was being issued for one-field-struct expected types (in which case it is misleading and outright wrong), even though it was only intended for one-field enum-variants (most notably, `Some`). Add a conditional to adhere to the original intent. (It would be possible to generalize to structs, but not obviously net desirable.) This adds a level of indentation, so the diff here is going to be easier to read in [ignore-whitespace mode](rust-lang@b0d3d3b9?w=1). Resolves rust-lang#55250. r? @pnkfelix
only issue "variant of the expected type" suggestion for enums This suggestion (introduced in pull-request rust-lang#43178 / eac7410) was being issued for one-field-struct expected types (in which case it is misleading and outright wrong), even though it was only intended for one-field enum-variants (most notably, `Some`). Add a conditional to adhere to the original intent. (It would be possible to generalize to structs, but not obviously net desirable.) This adds a level of indentation, so the diff here is going to be easier to read in [ignore-whitespace mode](rust-lang@b0d3d3b9?w=1). Resolves rust-lang#55250. r? @pnkfelix
only issue "variant of the expected type" suggestion for enums This suggestion (introduced in pull-request rust-lang#43178 / eac7410) was being issued for one-field-struct expected types (in which case it is misleading and outright wrong), even though it was only intended for one-field enum-variants (most notably, `Some`). Add a conditional to adhere to the original intent. (It would be possible to generalize to structs, but not obviously net desirable.) This adds a level of indentation, so the diff here is going to be easier to read in [ignore-whitespace mode](rust-lang@b0d3d3b9?w=1). Resolves rust-lang#55250. r? @pnkfelix
Following @est31's suggestion.
Resolves #42764.