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

early param count check for overloads #21673

Closed
wants to merge 9 commits into from
Closed

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Apr 15, 2023

closes nim-lang/RFCs#402, closes #18618 (this was adapted and improved), closes #14827, closes #19556, closes #20274 (all tested), closes #17164 (untested because implementation detail)

Store the minimum "required" parameters (without default values, varargs or void types) of routines in the position field of their symbols (could be attached to the type instead but there is no space in TType), and whether or not they have a varargs parameter into the type flag tfHasVarargsParam of the routine type.

Then use these along with tfVarargs (C-style {.varargs.}) to check, before any parameter match, to see if the given parameter count in a call expression is compatible with the parameter count of the routine.

This makes untyped parameters evade type checking in some cases (hence the linked issues) but also has a general benefit of ending the match check way earlier.

Disabled for nimsuggest because it uses the old behavior to give suggestions for partial calls, however I don't know if there is a way to disable this only when giving suggestions for partial calls.

Also legacy switch for old behavior

@metagn metagn changed the title test early param count check for overloads early param count check for overloads Apr 15, 2023
@metagn metagn marked this pull request as ready for review April 15, 2023 15:14
@metagn metagn marked this pull request as draft April 15, 2023 17:56
@metagn metagn marked this pull request as ready for review April 15, 2023 21:44
@Araq
Copy link
Member

Araq commented Apr 16, 2023

Wrong idea IMHO. What we need to do instead is to abolish untyped parameters as they cause Nim's macros to not compose. That means that things like

template withX(body: typed) = body

need to work reliably. Then we can simply deprecate untyped and all of the sudden the language works so much better and with fewer rules.

@metagn
Copy link
Collaborator Author

metagn commented Apr 16, 2023

I mean the way the parameter type checking works that would still have the same problem, we need to force not fully compiled expressions to be fully compiled to check if they match concrete types (which always match better)

The problem is when it is used in places the type system should easily be able to handle without tons of new constructs and complexity, which is not always the case. I wouldn't know how to reasonably define std/with or => with the type system

We can deal with the most common cases, like iterable (which is the language's responsibility, and there is a way better design for than we have), or explicit inject signatures/template lambdas at the callsite (which you actually can do with a library but not reliably using the type system), or whatever.

Also I don't see anything wrong with this patch in general, for calls like - we have to match the types half as many times over like 30 overloads.

@Araq
Copy link
Member

Araq commented Apr 17, 2023

Also I don't see anything wrong with this patch in general, for calls like - we have to match the types half as many times over like 30 overloads.

That's only true for unary/binary plus and minus, most operations that are heavily overloaded like == don't differ in the number of parameters. It's just more logic added to the compiler mitigating the effects of untyped without really solving the issue.

I wouldn't know how to reasonably define std/with or => with the type system

Not a problem, typed exists, it's not like you need to solve some new type system riddles here, all we need to have is a typed that cannot fail. It's a sort of "best effort" sem'checking that can leave nkIdent if not declared and maybe even make usage of nkError. It's the job of the macro to handle nkIdent/nkError or else the semantic checking after the macro expansion will fail.

@metagn
Copy link
Collaborator Author

metagn commented Apr 17, 2023

Thinking about it more, outside of the right-justified untyped parameters the language has (block arguments, macro pragma bodies etc) which change position depending on the param count, I can think of little use cases for untyped parameters being overloaded in the same position with typed parameters (the "real problem").

The flexibleOptionalParams thing also focuses on this but in a more restrictive way, only accounting for nkStmtList (colon blocks). and not the other possibilities (like other block arguments, or macro pragma bodies in #20274).

Maybe there's a way to change the design of "add blocks/pragma macro defs to the end of the call" that is compatible with the existing style of overloading, maybe new types like BlockArgument, Else, ProcDef that are ignored during overloading and some annotation (maybe simple enough as a node kind like nkStmtList) that delineates "this argument should not be typed", or maybe a separate param list for just these kinds of arguments, or maybe we do generalize all untyped to typed. Or we could add 30 lines of logic to the compiler to handle most issues people have. Not my call, honestly I don't have a preference, but I can promise I have enough sense of responsibility to fix any problems it (or I) might cause

@Araq
Copy link
Member

Araq commented Apr 17, 2023

The "real" problem: Put await in a template and see if the async macro can handle it. Nothing academic about the problem that macros don't compose. And some form of typed is our only chance of making it work.

@metagn
Copy link
Collaborator Author

metagn commented Apr 21, 2023

I really think this is fine but whatever, I'm not going to maintain this PR if no one wants it. (For the record await actually is a template right now)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment