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

restrict usage of typedesc, improve error messages #11959

Closed
wants to merge 8 commits into from

Conversation

krux02
Copy link
Contributor

@krux02 krux02 commented Aug 15, 2019

Restrict the usage of typedesc for parameters only.

code like this won't compile anymore:

proc foo(arg: int): typedesc =
  return float

This PR will also introduce a warning that encourages to rewrite code like proc foo(T: typedesc): T as proc foo[T](t: typedesc[T]): T. This way of passing type parameters to procedures has been proven to be the most reliable way to do it.

@krux02 krux02 requested a review from zah August 16, 2019 11:17
@krux02
Copy link
Contributor Author

krux02 commented Aug 16, 2019

@zah I am requesting your review of this PR, since you were involved in designing typedesc as it is right now. I think the design to have typedesc as value was a nice experiment that did not turn out to be useful for anything. Disabling it allows me to have a generally stricter compiler and therefore also nicer error messages.

I am also interested in your perspective because you are most likely to know some use cases that were not covered in the test cases that I broke with this PR.

@krux02
Copy link
Contributor Author

krux02 commented Aug 18, 2019

I don't know how if I can say this PR would fix #11152. This PR disallows/discourages the pattern in the named PR in such a way that the problem would not occur. In general this PR make the uglyness of typedesc bubble up. But on the upside at least you get a grasp of what typedesc is actually doing in the compiler.

@arnetheduck
Copy link
Contributor

Here's an example that "could" be a proc if types where first-class citizens all the way (actually haven't tried to use it in this case but...):

https://github.com/status-im/nim-stint/blob/master/stint/private/datatypes.nim#L133

reasoning about types is generally useful when dealing with generics etc - the less "friction"
and special syntax for that, the more the language will feel natural to use at compile time imo (instead of having to deal with the kludgy and unergonomic macros for example)

@krux02
Copy link
Contributor Author

krux02 commented Aug 19, 2019

@arnetheduck I am sorry I have no Idea what you are talking about. What do you mean by "types where first-class citizens"? I don't know what you mean with "could" in quotes. And I don't see how the example you give would be any better if it would be a proc. So what is your suggestion?

@andreaferretti
Copy link
Collaborator

@krux02 Why do you say this PR discourages the pattern in #11152? In that example, typedesc is used as a parameter, which this PR still allows.

@krux02
Copy link
Contributor Author

krux02 commented Aug 19, 2019

proc f[T](X: typedesc) is currently transformed into proc f[T, Hidden1](X: typedesc[Hidden1]) without telling the user about it. Our users don't know this. I've talked to people in the chat that said, "no I don't have a generic function, I use the non generic typedesc argument". This does not match the principle of least surprise at all. Procedures that take a typedesc parameter should do it with an explicit generic argument. It will look a bit more ugly in their declaration, but it will also be less surprising.

@andreaferretti
Copy link
Collaborator

I did not know either, but I agree that a proc taking a typedesc is generic, in the same way a proc taking a static parameter is: it cannot be compiled until it is specialized to know what the type (resp. the static value) is

@zah
Copy link
Member

zah commented Aug 19, 2019

This means procedures such as proc foo(T: typedesc): T have to be written as proc foo[T](t: typedesc[T]): T

I find this change completely pointless. typedesc is not the only construct in Nim that leads to implicitly inserted generic parameters. Any other type class appearing in a proc signature will produce the same behavior. #11152 could have been fixed in a more general way that would work for all type classes in a consistent way.

Crippling the syntax of type parameters because you personally don't like them and asking everyone to spend hours in non-productive code editing is a pretty good way to alienate the existing user base of Nim. I would suggest spending some time in going through our codebase to see how the required rework will make you feel.

Furthermore, this is yet another change that hasn't been reflected in the manual. Going through the required changes in the manual is a good way to see if a suggested change is conceptual simplification or whether it must be described as an unusual special case. At the moment, the manual describes typedesc as just another type class that works like any other generic type. I'd like to see how you'll reword it and justify the restricted usage here.

@krux02 krux02 force-pushed the restrict-typedesc branch from c7e871e to 6757b75 Compare August 19, 2019 21:15
@krux02
Copy link
Contributor Author

krux02 commented Jan 28, 2020

fixes #13225

@krux02 krux02 mentioned this pull request Feb 12, 2020
@stale
Copy link

stale bot commented Jan 27, 2021

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Jan 27, 2021
@stale stale bot closed this Feb 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants