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

Syntax improvement for ResultQuery usage #259

Open
Tracked by #264
KiChjang opened this issue Aug 24, 2022 · 5 comments
Open
Tracked by #264

Syntax improvement for ResultQuery usage #259

KiChjang opened this issue Aug 24, 2022 · 5 comments
Labels
D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I3-annoyance The node behaves within expectations, however this “expected behaviour” itself is at issue. I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@KiChjang
Copy link
Contributor

KiChjang commented Aug 24, 2022

paritytech/substrate#11257 introduces ResultQuery, which contain syntax that is in fact non-standard Rust when trying to use it as the QueryKind type parameter for a storage map. Example:

pub type Map3<T> = StorageMap<
	_,
	Blake2_128Concat,
	u32,
	u64,
	ResultQuery<Error<T>::NonExistentStorageValue>, // <---- This is non-standard Rust syntax
>;

The reason here is because Error<T>::NonExistentStorageValue is not a type, but rather a value, as enum variants are currently not properly recognized as their own proper types in Rust.

In addition, such a syntax does not cater to enum variants that accept fields, so implementors are forced to also use the OnEmpty parameter to specify what the default Result value should be when the corresponding value doesn't exist under the storage key specified.

We'd like to try and stick with standard Rust syntax as much as possible, and as a stretch goal, also allow for the flexibility of specifying the default query value without the usage of OnEmpty.

@bkchr
Copy link
Member

bkchr commented Aug 24, 2022

In addition, such a syntax does not cater to enum variants that accept fields, so implementors are forced to also use the OnEmpty parameter to specify what the default Result value should be when the corresponding value doesn't exist under the storage key specified.

I don't get this? What you mean by this?

TBH, I don't get the entire issue. You want to improve the syntax, but from what to what? What kind of syntax do you would like to see?

I mean yes, this is currently not standard Rust and I don't see it being transformed to standard rust in the near future if we don't want to have very complicated syntax.

Why can we not accept error variants that have fields? As long as these fields are const initialized?

@KiChjang
Copy link
Contributor Author

There's only a finite amount of customization that we can make on top of the standard Rust syntax, before it gets too strange and hurts discoverability and hence usage. We can't always rely upon documentation to expose certain programming features that pallets support, and so the more we deviate from standard Rust, the more unexpected syntactical constructs that we have to maintain and explain.

If we want Substrate to be the prime tool for people to use to build blockchains, then devex is a goal that we should strive for. The existence of customized syntax like this one makes it impossible for us to continue to claim that "if you know Rust, you know Substrate", as we're clearly creating our own dialect of Rust.

Functionality-wise, we can indeed parse any syntax and make things work, but that is not what this issue is focused on. We need to reduce our strangeness factor.

@bkchr
Copy link
Member

bkchr commented Aug 25, 2022

it impossible for us to continue to claim that "if you know Rust, you know Substrate",

Never heard this claim since I work at Substrate ;)

Nevertheless, back to the topic. Could you give an example from current syntax to what syntax you would like to change?

@KiChjang
Copy link
Contributor Author

KiChjang commented Sep 8, 2022

At the very least, I think the ResultQuery<Error::ErrorVariant> syntax has to be changed, most likely to a more idiomatic ResultQuery<Error> syntax.

Then the question becomes "how do we specify the error variant to use for the ResultQuery, and I think there are several idiomatic methods of doing so: the first one that I can immediately think of is to ensure that the OnEmpty type parameter exists when ResultQuery is specified, and we make use of the OnEmpty type parameter to specify which error variant to use.

@bkchr
Copy link
Member

bkchr commented Sep 8, 2022

"idiomatic" is just such a random word in Rust that everyone uses, but that has actually no real meaning.

OnEmpty would require that I create some extra type and implement the trait for it? This sounds like an awful user experience to me. Yes we are not here close to the Rust syntax. However, this may works in the future when you can have custom const values.

@juangirini juangirini transferred this issue from paritytech/substrate Aug 24, 2023
@the-right-joyce the-right-joyce added I3-annoyance The node behaves within expectations, however this “expected behaviour” itself is at issue. I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. and removed I4-annoyance labels Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I3-annoyance The node behaves within expectations, however this “expected behaviour” itself is at issue. I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
Status: Backlog
Development

No branches or pull requests

4 participants