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

deferImport: allows cyclic dependencies #18251

Closed
wants to merge 3 commits into from

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Jun 13, 2021

  • straightforward implementation (most added files are just tests)
  • this helps dealing with cyclic dependencies in many cases, avoiding painful contorsions / hacks based on include or large refactorings
  • this relates to compilerproc but allows using this in user code instead of limiting it to compiler use

notes

after this PR, nim could avoid a lot of include files included from sem.nim, by using exportc proc; and, with future work noted below, the exportc requirement could also be lifted.

future work

provide a way to use cyclic dependency APIs without exportc/importc, by specifying which module the API comes from, so that the regular nim mangling applies; this should allow using overloaded procs, maybe even generic procs, and avoid caveats of C mangling.

@timotheecour timotheecour marked this pull request as ready for review June 13, 2021 00:53
@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label Jun 13, 2021
@alaviss
Copy link
Collaborator

alaviss commented Jun 13, 2021

Isn't one of the perks coming out of IC would be to enable cyclic imports? With IC nearing completion, why would we need this magic?

@timotheecour
Copy link
Member Author

IC would be to enable cyclic imports?

how does IC enable cyclic imports?

With IC nearing completion

Based on what evidence? compiling or partial recompiling with --incremental:on is still much slower than without --incremental:on, not to mention it errors with most files I try it on (eg, try tests/stdlib/tsequtils.nim); but I don't want to make this PR a discussion about IC, this is IMO a separate topic.

This PR solves real problems and simplifies working with cyclic dependencies.

@alaviss
Copy link
Collaborator

alaviss commented Jun 13, 2021

how does IC enable cyclic imports?

Based on what @Araq has been saying on multiple chat platforms for as long as IC was talked about. IC and the packedast that come with it was said to enable the implementation of cyclic imports and the removal of forward declaration.

Based on what evidence?

https://github.com/nim-lang/Nim/projects/1

There's only one TODO left.

This PR solves real problems and simplifies working with cyclic dependencies.

I don't want to solve language issues with hacks.

@Araq
Copy link
Member

Araq commented Jun 13, 2021

IC does not enable cyclic imports but I don't want to enable cyclic imports until we have IC as I don't want a solution that conflicts with IC. I don't see how we need deferImport in order to allow cyclic imports, the real solution to me seems to change the sem-checking phases.

@saem
Copy link
Contributor

saem commented Jun 13, 2021

Also, encourage better software engineering by separating interface and implementation(s). Having been subjected to code bases in languages that allow this I've invariably had to untangle the mess it creates.

@timotheecour
Copy link
Member Author

timotheecour commented Jun 13, 2021

Also, encourage better software engineering by separating interface and implementation(s). Having been subjected to code bases in languages that allow this I've invariably had to untangle the mess it creates.

uhm... do you realize that this PR actually enables separating interface and implementation for code that requires it? And that there is no cyclic import dependencies involved? It enables cyclic dependencies between APIs (procs from A calling procs from B and vice versa) without creating a cyclic import graph.

See tests, or this example:

# foo.nim
proc foo_fn1(){.importc.}
proc fn1*() = foo_fn1()
deferImport "foo_impl"

# foo_impl.nim
# implementation file; can contain more imports, even ones that import foo or main
proc foo_fn1(){.exportc.} = discard

# main.nim
import foo
fn1()

import graph after calling nim r main:
main => foo
foo_impl => {other modules, possilby main or foo}
=> no cycles

I don't want a solution that conflicts with IC. I don't see how we need deferImport in order to allow cyclic imports

no conflict with IC, if anything it's the opposite, it makes incremental compilation easier because it allows one to separate interface from implementation in cases where this is needed; if only the implementation changes and interface is unchanged, only the modules involved in implementation need to be recompiled. Again, this doesn't cause cyclic dependencies in the import graph

There's only one TODO left.

and what does that prove exactly? the TODO only concerns gc:arc and makes no mention of the other points i raised in #18251 (comment). It's great if IC eventually works, IC is a key feature (either via nimrod files or CAAS), but claiming it's nearly complete doesn't resonate with my experience.

real solution to me seems to change the sem-checking phases.

can you please elaborate?

@saem
Copy link
Contributor

saem commented Jun 13, 2021

uhm... do you realize that this PR actually enables separating interface and implementation for code that requires it?

Congratulations on missing the point. You're pointing to sem and separation of implementation and interface. This doesn't help it just encourages bad design.

What would help sem would be to slowly remove the includes and reshape the code such that it's not organized only by loose topic of proc but phases and lifecycles. The latter are much more difficult to model in ones head and far more useful than modules like "bag of generic handling stuff". Right now the code is arranged for rapid indexing of proc by topic, but seeing as go to definition has consistently gotten better along with symbol search this is less important.

Your minimized example doesn't make sense either, there are two classic cases:

  • an interface as a minimal surface for some consumer and some implementation for it
  • an interface as a protocol/data and a client and server implementations for it

There is no reason why foo needs to refer to foo impl's anything and some very straightforward rearrangement of code should address that. Your PR fails to provide a motivating example.

@Araq
Copy link
Member

Araq commented Jun 14, 2021

There is no reason why foo needs to refer to foo impl's anything and some very straightforward rearrangement of code should address that. Your PR fails to provide a motivating example.

That is not important though -- while many examples of "I need cyclic imports" can be fought with "no, you don't and it's a bad architecture" it is considered by me a key missing feature for multiple reasons:

  • People really want the feature and Nim's philosophy is to serve and not to prevent one from "getting things done".
  • More importantly: Cyclic imports can easily arise when wrapping large C++ code bases. Or when porting code from C# etc.

But as I said, allowing cyclic imports does not need to be done via deferImport.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Jul 7, 2022
@nim-lang nim-lang deleted a comment from stale bot Jul 7, 2022
@ringabout ringabout removed the stale Staled PR/issues; remove the label after fixing them label Jul 7, 2022
@Araq
Copy link
Member

Araq commented Jul 7, 2022

It's not deferImport that helps with cyclic dependencies. The mechanism actually uses importc/exportc to circumvent the cycle. The importc/exportc hack is already available though and "import X later" can easily be done by hacking "import" statements into your main.nim file. I don't see the point in this.

@Araq Araq closed this Jul 7, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants