Skip to content

Conversation

@jo-so
Copy link
Contributor

@jo-so jo-so commented Sep 1, 2021

Most commonly a field of a struct can be initialized with its default value than an empty tuple.

"#,
r#"
fn some(, b: () ) {}
fn some(, b: Default::default() ) {}
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's what the comment above says: FIXME: this is very wrong, but somewhat tricky to fix.

Copy link
Member

Choose a reason for hiding this comment

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

Ye that's the macro at fault, nothing specific to this PR

@bjorn3
Copy link
Member

bjorn3 commented Sep 1, 2021

I think it is harded to replace Default::default() than (). Could you at least check that the field type implements Default and use vec![] for Vec and None for Option? Maybe it would also be nice to use the only possible value directly for unit structs?

@jo-so
Copy link
Contributor Author

jo-so commented Sep 1, 2021

But with Default::default() the code compiles most of the time while it doesn't with (). And leaving Default::default() makes it easier when you refactor the structure, e.g. you change a Vec<…> into a HashSet<…>.

Maybe someone else can tell how he thinks about Default::default() vs. ().

@Veykril
Copy link
Member

Veykril commented Sep 1, 2021

We should check if it implements the necessary trait certainly, and ideally also make this a config option as replacing that default path expression is a lot more work than replacing () so I'm sure there are people who prefer () over that. cc #1470

@matklad
Copy link
Contributor

matklad commented Sep 1, 2021

This should be consitent with fill_match_arms first and foremost. That uses todo!() at the moment.

Either way, we should extract the code for "come up with default expression for type" into assist/utils, and use that in both places.

As to what's that expression is to be, I see the following choices:

  • () as the smallest thing
  • todo!() short, and semantically correct thing
  • default value for a type (0, None, HashMap::default(), depending on the type in question).

I think todo!() is probably the best choice -- it's reasonably compact, reasonably correct, and robust (it doesnt' depend on type of the thing, which is an advantage, as IDE always behaves predictably).

@matklad
Copy link
Contributor

matklad commented Sep 1, 2021

@Veykril
Copy link
Member

Veykril commented Sep 1, 2021

@jo-so are you interested in trying to implement this configurably as outlined by matklad? If yes feel free to ask questions here or on zulip, if not that's also fine!

@matklad
Copy link
Contributor

matklad commented Sep 1, 2021

To clarify -- I don't think we need configurability in this case, we should just pick a good choice. I am confident that todo!() is a reasonable choice, but if folks feel strongly that some other choice would be better, than yeah, we can make this configurable!

@flodiebold
Copy link
Member

I would prefer todo!() and kind of want to use todo!() as 'holes' with some editor functionality to switch between them and replace them easily.

@bjorn3
Copy link
Member

bjorn3 commented Sep 1, 2021

XCode has native support for placeholders. (search for placeholder in https://developer.apple.com/documentation/xcode/creating-organizing-and-editing-source-files) They are automatically inserted for arguments when autocompleting. They are saved as <#placeholder text#> and appear as blue rounded rectangles that can only be selected as a whole. You can navigate between them using tab and shift+tab.

Maybe it is possible to emulate it using the javascript api of vscode? Or do lsp replacement inlay hints act as a single selectable entity in vscode? As for the file syntax, while <#placeholder#> may be valid inside macros, I think it is sufficiently uncommon that there is value in using this format just like XCode.

@ruabmbua
Copy link
Contributor

ruabmbua commented Sep 6, 2021

First random thought on this: why not use todo!() instead? Will look as ugly as Default::default(), but work in (probably) all cases.

@jo-so
Copy link
Contributor Author

jo-so commented Sep 9, 2021

I've found a way to do this in Emacs. So, I'm fine with everything even dropping the PR.

The generated code with `()` doesn't compile in most of the cases. To signal
the developer there's something to do, fill in `todo!()`.

Because the file *missing_fields.rs* contains the string `todo!()` it needs
an exception for the test *check_todo*.
@jo-so jo-so changed the title RfC: Use Default::default() instead of () for missing fields RfC: Use todo!() instead of () for missing fields Sep 9, 2021
@jonas-schievink
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 24, 2021

@bors bors bot merged commit eb17e90 into rust-lang:master Sep 24, 2021
@lnicola
Copy link
Member

lnicola commented Sep 24, 2021

changelog fix (first contribution) use todo!() instead of () in "Fill missing fields"

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.

8 participants