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

fix nim-lang/RFCs#18; add reflection.moduleSymbols to list symbols in a module #9560

Closed
wants to merge 4 commits into from

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Oct 29, 2018

@timotheecour timotheecour changed the title add macros.moduleSymbols to get seq of symbols in a module add macros.moduleSymbols to list public symbols in a module Oct 29, 2018
@Araq
Copy link
Member

Araq commented Oct 30, 2018

I don't like the feature (the implementation mostly follows the existing pattern). Previously I told people to use parseStmt on a full file to get the AST. I'd like to preserve the following invariant in Nim code: Given mymacro: x, only x can be affected by the transformation that mymacro does. Your macros.moduleSymbols breaks this invariant, local reasoning about Nim code gets harder.

@krux02
Copy link
Contributor

krux02 commented Nov 8, 2018

I am closing as rejected. Adding API's like this needs careful thinking instead of exposing all internal structures.

@krux02 krux02 closed this Nov 8, 2018
@timotheecour timotheecour changed the title add macros.moduleSymbols to list public symbols in a module [TODO] add macros.moduleSymbols to list public symbols in a module Nov 19, 2018
@sighoya
Copy link

sighoya commented Jun 7, 2021

Question: Can we open up it again, I find this very useful.
Essentially, it replaces the need for custom builder/compiler plugins to do "reflection".
What you know in Java with annotations could then be realized in Nim with pragmas at compile time.
Examples:

  • infer all children of a class like type
  • first class dynamic dispatching by retrieving all overloads
  • non-local dependency injection
  • generating all c/c++ wrappers automatically by traversing all importcpp pragmas?

A disadvantage of this feature is the non-localness literally allowing any local code change to break your code, however at least at compile time.

@Araq wrote:

Given mymacro: x, only x can be affected by the transformation that mymacro does.

Understand, so assignments could be created for non local vars. But I think bindSym hackery already allows such things, okay dynamically scoped bindSyms are experimental yet.

@timotheecour timotheecour reopened this Jun 7, 2021
@timotheecour timotheecour marked this pull request as draft June 7, 2021 21:14
@timotheecour
Copy link
Member Author

timotheecour commented Jun 7, 2021

Question: Can we open up it again, I find this very useful.

yes, it is very useful and an often requested feature (see some links i added on top), I've finally re-opened this PR and updated it.

@Araq PTAL

refs https://forum.nim-lang.org/t/1817#37522

It's been 4 years (EDIT: now 5), I cannot count how often I got this feature request. What does that mean? If a really suitable implementation comes along I won't fight it as hard anymore. Maybe.

update since original PR:

  • rebased, fixed conflicts and updated implementation
  • moved this from std/macros to std/reflection, where it belongs
  • added more illustrative runnableExamples
  • added enablePrivate = false optional param (follows import {.all.} PR)

why std/reflection

this is the most sensible place for both moduleSymbols (this PR) and the other reflection routines from #17641

#17641 (comment)
The name std / reflection is a recipe for bloat. Reflection is a wide field and many people expect there to exist RTTI related functionality. I propose std / sourceinfo instead.

nim is a static compiled language, so CT being implied by reflection will surprise no-one; in any case I've added a doc comment to make it clear that the scope of this module is CT reflection. For RT reflection / RTTI, we already have a module, https://nim-lang.github.io/Nim/typeinfo.html

std/sourceinfo doesn't reflect the intended scope as well as std/reflection.

why not parseStmt(readFile("module.nim"))

it suffers from the same pitfalls as include; you get different symbols from the ones actually used in the project. You can still use parseStmt(readFile("module.nim")) if you want, but the semantics are different and usually not what a user interested in reflection wants.

why not use compilerapi ?

compilerapi (which few people use anyway) is no replacement for a reflection module and moduleSymbols in particular; it doesn't have the ease of use of nim macros and is a meta-api that doesn't integrate easily with user code.

other languages have reflection too:

honestly, it's a surprising omission in nim; std/macros doesn't replace the need for std/reflection which can be very useful for user code.

@timotheecour timotheecour force-pushed the pr_moduleSymbols branch 2 times, most recently from 422beea to 36cde6d Compare June 8, 2021 02:27
@timotheecour timotheecour changed the title [TODO] add macros.moduleSymbols to list public symbols in a module add reflections.moduleSymbols to list symbols in a module Jun 8, 2021
@timotheecour timotheecour changed the title add reflections.moduleSymbols to list symbols in a module fix nim-lang/RFCs#18; add reflections.moduleSymbols to list symbols in a module Jun 8, 2021
@timotheecour timotheecour marked this pull request as ready for review June 8, 2021 02:42
@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label Jun 8, 2021
@timotheecour timotheecour changed the title fix nim-lang/RFCs#18; add reflections.moduleSymbols to list symbols in a module fix nim-lang/RFCs#18; add reflection.moduleSymbols to list symbols in a module Jun 8, 2021
@timotheecour
Copy link
Member Author

timotheecour commented Jul 14, 2021

ping @Araq, see updates in #9560 (comment)

@Varriount
Copy link
Contributor

Is there a general overview of what reflection capabilities Nim should have. and what are feasible? While I think this has merit, modules aiming to serve a functionality like "reflection" are best designed as a whole, rather than piecemeal. For example, see how https://blog.golang.org/laws-of-reflection and Go's reflection package work.

@Araq
Copy link
Member

Araq commented Jul 23, 2021

Is there a general overview of what reflection capabilities Nim should have. and what are feasible?

It's pretty simple: Nim should never have reflection like Java and C# because Dead-code elimination becomes fragile or impossible. In general, musing about what would harm an optimizer is a good design principle if "easy to reason about" is not just a buzzword phrase.

@timotheecour
Copy link
Member Author

timotheecour commented Jul 25, 2021

It's pretty simple: Nim should never have reflection like Java and C# because Dead-code elimination becomes fragile or impossible. In general, musing about what would harm an optimizer is a good design principle if "easy to reason about" is not just a buzzword phrase.

@Araq I fail to see how this PR impacts dead code elimination in any way. moduleSymbols only lists the symbols in a module; if they're not called from a top-level scope, dead-code elimination rules still apply exactly as before this PR. Listing properties (eg declaration line, doc comments, etc) derived from all symbols in a module also won't impact DCE.

Example

nim r -d:case4a main # no stdlib_strutils.nim.c generated
nim r -d:case4a main # stdlib_strutils.nim.c generated but only CmpIgnoreCase is codegen'ed
when true:
  import std/[macros, reflection, strutils]
  macro moduleSymbolsTest(mymod: typed, enablePrivate: static bool = false): untyped =
    for ai in mymod.moduleSymbols(enablePrivate):
      if $ai == "cmpIgnoreCase": return ai
  when defined case4a:
    const foo1 = moduleSymbolsTest(strutils, enablePrivate = true)
    echo foo1("ab", "Ab")
  when defined case4b:
    let foo2 = moduleSymbolsTest(strutils, enablePrivate = true)
    echo foo2("ab", "Ab")

The feature is useful, and has been requested many, many times over; can we please merge this?

Most other languages I've used (D, go, java, python3, js, C#, ruby ...) allow this kind of reflection APIs, those languages didn't stop being analyzable or optimizable because of this; it's a very useful thing to have when you need it.

@Araq
Copy link
Member

Araq commented Jul 26, 2021

@Araq I fail to see how this PR impacts dead code elimination in any way.

I didn't mean to imply that it does impact it. I merely replied to @Varriount and outlined some of the design principles that we follow.

@timotheecour
Copy link
Member Author

timotheecour commented Oct 5, 2021

ping @Araq this keeps coming up, eg https://discord.com/channels/371759389889003530/768367394547957761/894724802185687071 (for nimscripter)

@deech
Copy link
Contributor

deech commented Oct 25, 2021

This would be very useful to me, I want to extern the exposed types/procs in a module compiled with the cpp backend so it can be used from other languages. Having a way to simply list the public contents of a module would make this a lot easier. We already have std/importutils so this seems like the next logical step.

@Araq
Copy link
Member

Araq commented Oct 25, 2021

In order to proceed

  1. rename it to something else, reflection typically is used for runtime reflection.
  2. Make it clear and ideally enforce it that the resulting sequence of NimNodes cannot be mutated. Or that mutations do not affect the compilation pipeline.

@stale
Copy link

stale bot commented Oct 28, 2022

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 Oct 28, 2022
@stale stale bot closed this Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review (please take another look): ready for next review round stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: macros able to iterate over symbols
6 participants