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

Make usage of quotes in errors consistent #7578

Closed
Marenz opened this issue Oct 28, 2019 · 28 comments
Closed

Make usage of quotes in errors consistent #7578

Marenz opened this issue Oct 28, 2019 · 28 comments
Labels
closed due inactivity The issue/PR was automatically closed due to inactivity. easy difficulty good first issue candidate Could be a "good first issue" but something is blocking it or it has open questions. stale The issue/PR was marked as stale because it has been open for too long.

Comments

@Marenz
Copy link
Contributor

Marenz commented Oct 28, 2019

Our error messages are not consistent regarding the use of quotes, e.g. some quote the function name and contract name, others don't.

We should agree on how we want to do that and make it consistent everywhere.

@erak argues for double quotes " around all user defined names.
I tend to agree.

@birtony
Copy link

birtony commented Oct 29, 2019

Hey, I would be happy to improve the error messages!

@Marenz
Copy link
Contributor Author

Marenz commented Oct 29, 2019

Glad to hear that!

We still have to decide on how exactly this should be done, which we probably will do in tomorrows meeting. Feel free to join tomorrow (Wednesday at 15 CET, a link will be posted in gitter) in our google hangout meeting if you want to talk about it.

In any case I'll update this issue with the result of the discussion!

@birtony
Copy link

birtony commented Oct 29, 2019

Thank you, @Marenz!

@birtony
Copy link

birtony commented Oct 30, 2019

Hey @Marenz! Unfortunately I could not join you for the meeting today. Have you been able to reach any agreement on this issue?

@Marenz
Copy link
Contributor Author

Marenz commented Oct 31, 2019

Yes. We'd like for all user defined names to be in quotes in all error messages. Sorry I didn't update sooner.

@birtony
Copy link

birtony commented Oct 31, 2019

In single or in double quotes?

@Marenz
Copy link
Contributor Author

Marenz commented Nov 4, 2019

Double quotes

@birtony
Copy link

birtony commented Nov 21, 2019

I was looking into the error messages, and I have also noticed, that a lot of custom defined errors are used. The problem is that I am not totally sure, which exact errors should be changed. Could you, please, provide me with an example of any error that has "user defined names" within it that needs to be updated?

@Marenz
Copy link
Contributor Author

Marenz commented Nov 26, 2019

Here are a few examples for you: :)

// Uses single quotes instead of double quotes
m_errorReporter.warning(_statement.location(), "Failure condition of 'send' ignored. Consider using 'transfer' instead.");
// Should be this
m_errorReporter.warning(_statement.location(), "Failure condition of \"send\" ignored. Consider using \"transfer\" instead.");

here no quotes at all are used:

m_errorReporter.typeError(
        _statement.location(),
        errorMsg +
        ". Try converting to type " +
        valueComponentType->mobileType()->toString() +
        " or use an explicit conversion."
);
// Should be this
m_errorReporter.typeError(
        _statement.location(),
        errorMsg +
        ". Try converting to type \"" +
        valueComponentType->mobileType()->toString() +
        "\" or use an explicit conversion."
);


 m_errorReporter.fatalTypeError(_tuple.location(), "Type " + inlineArrayType->toString() + " is only valid in storage.");
// Should be this
 m_errorReporter.fatalTypeError(_tuple.location(), "Type \"" + inlineArrayType->toString() + "\" is only valid in storage.");

@kalashshah11
Copy link

Hey, @Marenz Is this still left to work upon? I would be glad to contribute.

@birtony
Copy link

birtony commented Dec 7, 2019

@kalashshah11 I am going to push an update in a bit with all the updated error messages I could've found

@axic
Copy link
Member

axic commented Dec 8, 2019

Why not introduce a toQuoted or toQuotedString helper?

@leonardoalt
Copy link
Member

@axic such as #7733 ? ;)

@axic
Copy link
Member

axic commented Dec 8, 2019

Can't we just implement our alternative in 3 lines of code?

@leonardoalt
Copy link
Member

Sure, that'd work

@Marenz
Copy link
Contributor Author

Marenz commented Dec 9, 2019

When @erak and I talked about it we decided to make it all consistent as a first step and as a second step use a function like that.

@axic
Copy link
Member

axic commented Dec 9, 2019

Sounds like a lot of extra work 😉

@Marenz
Copy link
Contributor Author

Marenz commented Dec 9, 2019

I argued along similar lines and I forgot eraks counter-argument ;)

@erak
Copy link
Collaborator

erak commented Dec 9, 2019

Well, I think I had no strong opinion on the process ;-) Would agree with @axic here and start using the helper function right away.

@Marenz
Copy link
Contributor Author

Marenz commented Dec 9, 2019

What does that mean in regards to the pending PR?

@antara-gandhi
Copy link

Hello! I'm new to contributing to open-source, and I saw this issue was still open. It looks like it should be resolved to me - is there still something that needs to be done that I could help out with?

@cameel
Copy link
Member

cameel commented Aug 16, 2021

@antara-gandhi We still do not have the helper and quoting in error messages is not used consistently :) Well, we do have escapeAndQuoteString() and I've seen it used for quoting sometimes but I'm not sure we really want escaping there and the name is a bit long for something to be used as often as quoting.

So my suggestion would be to start by creating the helper in CommonData.h, then find some messages and use it in them.

@chriseth
Copy link
Contributor

I'm not sure the helper is that helpful (haha), we certainly do not want to escape in these cases.

@cameel
Copy link
Member

cameel commented Aug 16, 2021

Well, it's an old issue so maybe the sentiment changed here :)

Personally, I'd prefer something like

m_errorReporter.typeError(
        _statement.location(),
        errorMsg +
        ". Try converting to type " +
        quoted(valueComponentType->mobileType()->toString()) +
        " or use an explicit conversion."
);

over

m_errorReporter.typeError(
        _statement.location(),
        errorMsg +
        ". Try converting to type \"" +
        valueComponentType->mobileType()->toString() +
        "\" or use an explicit conversion."
);

or we could switch to fmtlib: #11273 (comment) :)

@chriseth
Copy link
Contributor

I think the second version makes it much clearer what is between the strings. For the one with quoted you first have to read quoted and discard it as non-essential.

@cameel
Copy link
Member

cameel commented Aug 17, 2021

But with escaped quotes you have to parse them visually and discard as non-essential :) When I see that when skimming over the code I just skip it as something complex not worth analyzing closer while in the version with quoted() I can easily see that it's just an expression between two strings.

Anyway, I do not feel very strongly about this issue and I'm fine with either, just want to have a decision here so that contributors can work on it. Looks like no one was against quoting itself and it's just a matter of how we insert them (with or without a helper) so to cut the discussion short, I think it would be fine to start working on this by just inserting quotes directly. We can add a helper later if we get some consensus here.

@cameel cameel added good first issue candidate Could be a "good first issue" but something is blocking it or it has open questions. and removed good first issue labels Dec 5, 2022
@github-actions
Copy link

github-actions bot commented Mar 6, 2023

This issue has been marked as stale due to inactivity for the last 90 days.
It will be automatically closed in 7 days.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Mar 6, 2023
@github-actions
Copy link

Hi everyone! This issue has been automatically closed due to inactivity.
If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen.
However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.

@github-actions github-actions bot added the closed due inactivity The issue/PR was automatically closed due to inactivity. label Mar 13, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed due inactivity The issue/PR was automatically closed due to inactivity. easy difficulty good first issue candidate Could be a "good first issue" but something is blocking it or it has open questions. stale The issue/PR was marked as stale because it has been open for too long.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants