Make module reference directives less confusing

-- Problem

The way module references are implemented was a big source of confusion,
mainly because in most cases there are at least two possible ways they
are handled. Additionally, for some modules (checks and modifiers) there
is the third way, what doesn't help either.

Consider the following cases of confiugraiton directives:
```
deliver_to smtp_downstream
```
This directive refers to the existing configuration block named
smtp_downstream. It doesn't have to be the instance of the
smtp_downstream module, though.

```
deliver_to smtp_downstream tcp://127.0.0.1:1125
```
This magically turns "reference to an existing block" into an inline
definition. And suddenly the first argument is not an configuration
block name, now it is a module name. Same "sudden" change happens when
the block is added:
```
deliver_to smtp_downstream { ... }
```

For modules having an "implicitly defined" config block, there's
another source of confusion:
```
deliver_to smtp_downstream
```
This directive may refere to the implicitly defined config block with
some default values. But to the user it looks like a module name and
nothing more. It's trickly to explain all dark corners of such behavior
in the user documentation.

Even more, there's another problem with the implementation, these
implicitly defined blocks can't access global directives because they
are defined before the configuration is parsed.

-- Solution

This commit removes the third way of module reference handling. There
are no "implicitly defined" config blocks anymore.

Second, all module references by default create a new module instance.

All following directives create a new module instance, no catches here.
```
deliver_to smtp_downstream
deliver_to smtp_downstream tcp://127.0.0.1:2525
deliver_to smtp_downstream { ... }
```
Although, the first one will fail because smtp_downstream needs at least
one endpoint address somewhere.

Ability to define configuration blocks at a top-level and reference them
in other places is retained for use in cases where it's actually
useful, including the initial idea of "state sharing" (see "Dev:
Comments on design" on the project wiki).

However, such referneces are now explicitly prefixed by the '&'
character. Such as the following:
```
deliver_to &smtp_downstream
```
This directive references the existing configuration block named
"smtp_downstream". Following directives are not allowed as they make no
sense:
```
deliver_to &smtp_downstream tcp://127.0.0.1:2525
deliver_to &smtp_downstream { ... }
```

So, there is no confusion about what happens when.

Closes foxcpp#167. I decided to not make any radical changes now. Changes
made to the initialization logic solve the actual problem that led to
the creation of the referenced issue.

HACKING.md

@@ -25,7 +25,7 @@
   Allow connecting components in arbitrary ways instead of restricting users to
   predefined templates.
 
-- **Use Go concurrency features to a full extent.**
+- **Use Go concurrency features to the full extent.**
   Do as much I/O as possible in parallel to minimize latencies. It is silly to
   not do so when it is possible.
 
@@ -43,38 +43,23 @@ well.
 There are components called "modules". They are represented by objects
 implementing the module.Module interface. Each module gets its unique name.
 The function used to create a module instance is saved with this name as a key
-into the global map called "modules registry". Each module can be created
-multiple times, each instance gets its unique name and is placed into a global
-map (a separate one) too.
-
-Modules can reference each other by instance names (module.GetInstance). When a
-module instance reference is acquired, the caller usually checks whether the
-module in question implements the needed interface. Module implementers are
-discouraged from using module.GetInstance directly and should prefer using
-ModuleFromNode or config.Map matchers. These functions handle "inline module
-definition" syntax in addition to simple instance references.
-
-Module instances are initialized lazily if they are referenced by other modules
-(module.GetInstance calls mod.Init if necessary). Module instances not
-referenced explicitly anywhere but still defined in the configuration are
-initialized in arbitrary order by the Start function (below).
-
-Module instances that are defined by the code itself ("implicitly defined") may
-be left uninitialized unless they are used.
-
-A single module instance can have one or more names. The first name is called
-"instance name" and is the primary one, it is used in logs.  Other names are
-called "aliases" and only used by module.GetInstance (e.g. module instance can
-be fetched by any name).
-
-Some module instances are defined "inline" in user configuration. That is.
-their configuration block is placed right where they are used. These instances
-have no instance names and are not added to the global map so they can not be
-accessed by modules other than one that used ConfigFromNode on the
-corresponding config block. All arguments after the module name in an inline
-definition represent "inline arguments". They are passed to the module instance
-directly and not used anyhow by other code (i.e. they are not guaranteed to be
-unique).
+into the global map called "modules registry". 
+
+Whenever module needs another module for some functionality, it references it
+using a configuration directive with a matcher that internalls calls
+modconfig.ModuleFromNode. That function looks up the module "constructor" in
+the registry, calls it with corresponding arguments, checks whether the
+returned module satisfies the needed interfaces and then initializes it.
+
+Alternatively, if configuration uses &-syntax to reference existing
+configuration block, ModuleFromNode simply looks it up in the global instances
+registry. All modules defined the configuration as a separate top-level blocks
+are created before main initialization and are placed in the instances registry
+where they can be looked up as mentioned before.
+
+Top-level defined module instances are initialized (Init method) lazily as they
+are required by other modules. 'smtp' and 'imap' modules follow a special
+initialization path, so they are always initialized directly.
 
 ## Error handling
 

check/dkim/dkim.go

@@ -232,8 +232,4 @@ func (c *Check) CheckStateForMsg(msgMeta *module.MsgMetadata) (module.CheckState
 
 func init() {
 	module.Register("verify_dkim", New)
-	module.RegisterInstance(&Check{
-		instName: "verify_dkim",
-		log:      log.Logger{Name: "verify_dkim"},
-	}, &config.Map{Block: &config.Node{}})
 }

check/spf/spf.go

@@ -284,8 +284,4 @@ func (s *state) Close() error {
 
 func init() {
 	module.Register(modName, New)
-	module.RegisterInstance(&Check{
-		instName: modName,
-		log:      log.Logger{Name: modName},
-	}, &config.Map{Block: &config.Node{}})
 }

check/stateless_check.go

@@ -163,24 +163,4 @@ func RegisterStatelessCheck(name string, defaultFailAction FailAction, connCheck
 			bodyCheck:   bodyCheck,
 		}, nil
 	})
-
-	// Here is the problem with global configuration.
-	// We can't grab it here because this function is likely
-	// called from init(). This RegisterInstance call
-	// needs to be moved somewhere after global config parsing
-	// so we will be able to pass globals to config.Map constructed
-	// here and then let Init access it.
-	// TODO.
-
-	module.RegisterInstance(&statelessCheck{
-		modName:  name,
-		instName: name,
-		resolver: net.DefaultResolver,
-		logger:   log.Logger{Name: name},
-
-		connCheck:   connCheck,
-		senderCheck: senderCheck,
-		rcptCheck:   rcptCheck,
-		bodyCheck:   bodyCheck,
-	}, &config.Map{Block: &config.Node{}})
 }

config/module/modconfig.go

@@ -11,9 +11,11 @@ package modconfig
 import (
 	"fmt"
 	"reflect"
+	"strings"
 
 	"github.com/foxcpp/maddy/config"
 	"github.com/foxcpp/maddy/config/parser"
+	"github.com/foxcpp/maddy/log"
 	"github.com/foxcpp/maddy/module"
 )
 
@@ -43,35 +45,30 @@ func initInlineModule(modObj module.Module, globals map[string]interface{}, bloc
 // args should contain values that are used to create module.
 // It should be either module name + instance name or just module name. Further extensions
 // may add other string arguments (currently, they can be accessed by module instances
-// as aliases argument to constructor).
+// as inlineArgs argument to constructor).
 //
 // It checks using reflection whether it is possible to store a module object into modObj
 // pointer (e.g. it implements all necessary interfaces) and stores it if everything is fine.
 // If module object doesn't implement necessary module interfaces - error is returned.
 // If modObj is not a pointer, ModuleFromNode panics.
 func ModuleFromNode(args []string, inlineCfg *config.Node, globals map[string]interface{}, moduleIface interface{}) error {
-	// single argument
-	//  - instance name of an existing module
-	// single argument + block
-	//  - module name, inline definition
-	// two+ arguments + block
-	//  - module name and instance name, inline definition
-	// two+ arguments, no block
-	//  - module name and instance name, inline definition, empty config block
-
 	if len(args) == 0 {
 		return parser.NodeErr(inlineCfg, "at least one argument is required")
 	}
 
+	referenceExisting := strings.HasPrefix(args[0], "&")
+
 	var modObj module.Module
 	var err error
-	if inlineCfg.Children != nil || len(args) > 1 {
-		modObj, err = createInlineModule(args[0], args[1:])
-	} else {
-		if len(args) != 1 {
-			return parser.NodeErr(inlineCfg, "exactly one argument is to use existing config block")
+	if referenceExisting {
+		if len(args) != 1 || inlineCfg.Children != nil {
+			return parser.NodeErr(inlineCfg, "exactly one argument is required to use existing config block")
 		}
-		modObj, err = module.GetInstance(args[0])
+		modObj, err = module.GetInstance(args[0][1:])
+		log.Debugf("%s:%d: reference %s", inlineCfg.File, inlineCfg.Line, args[0])
+	} else {
+		log.Debugf("%s:%d: new module %s %v", inlineCfg.File, inlineCfg.Line, args[0], args[1:])
+		modObj, err = createInlineModule(args[0], args[1:])
 	}
 	if err != nil {
 		return err
@@ -86,7 +83,7 @@ func ModuleFromNode(args []string, inlineCfg *config.Node, globals map[string]in
 
 	reflect.ValueOf(moduleIface).Elem().Set(reflect.ValueOf(modObj))
 
-	if inlineCfg.Children != nil || len(args) > 1 {
+	if !referenceExisting {
 		if err := initInlineModule(modObj, globals, inlineCfg); err != nil {
 			return err
 		}

dist/vim/syntax/maddy-conf.vim

@@ -31,10 +31,15 @@ syn match maddyInt '\<\d\+\>'
 syn match maddyInt '\<[-+]\d\+\>'
 syn match maddyFloat '\<[-+]\d\+\.\d*\<'
 
+syn match maddyReference /[ \t]&[^ \t]\+/ms=s+1 contains=maddyReferenceSign
+syn match maddyReferenceSign /&/ contained
+
 hi def link maddyBool		Boolean
 hi def link maddyInt		Number
 hi def link maddyFloat		Float
 
+hi def link maddyReferenceSign	Special
+
 " Module values
 
 " grep --no-file -E 'Register.*\(".+", ' **.go | sed -E 's/.+Register.*\("([^"]+)", .+/\1/' | sort -u

dummy.go

@@ -50,6 +50,7 @@ func (dd dummyDelivery) Commit() error {
 }
 
 func init() {
-	module.Register("dummy", nil)
-	module.RegisterInstance(&Dummy{instName: "dummy"}, nil)
+	module.Register("dummy", func(_, instName string, _, _ []string) (module.Module, error) {
+		return &Dummy{instName: instName}, nil
+	})
 }

examples/multitentant-dkim.conf

@@ -7,12 +7,12 @@ $(local_domains) = example.org example.com
     destination $(local_domains) {
         # Resolve aliases, etc... Put your stuff here.
 
-        deliver_to local_mailboxes
+        deliver_to &local_mailboxes
     }
 
     # Messages with non-local recipients to outbound queue.
     default_destination {
-        deliver_to remote_queue
+        deliver_to &remote_queue
     }
 }
 

examples/remote-aliases.conf

@@ -18,13 +18,13 @@ smtp tcp://127.0.0.1:25 {
         reroute {
             # If it is still a local recipient - deliver to local_mailboxes.
             destination $(local_domains) {
-                deliver_to local_mailboxes
+                deliver_to &local_mailboxes
             }
 
             # If it was aliased to a non-local recipient - put it into a queue
             # for outbound delivery.
             default_destination {
-                deliver_to remote_queue
+                deliver_to &remote_queue
             }
         }
     }

maddy.conf

@@ -70,7 +70,7 @@ smtp tcp://0.0.0.0:25 {
     # All messages for the recipients at $(local_domains) should be
     # delivered to local mailboxes.
     destination $(local_domains) {
-        deliver_to local_mailboxes
+        deliver_to &local_mailboxes
     }
 
     # Other recipients are rejected because we are not an open relay.
@@ -81,7 +81,7 @@ smtp tcp://0.0.0.0:25 {
 
 submission tls://0.0.0.0:465 {
     # Use sql module for authentication.
-    auth local_authdb
+    auth &local_authdb
 
     modify {
     	sign_dkim $(primary_domain) default
@@ -90,12 +90,12 @@ submission tls://0.0.0.0:465 {
     # All messages for the recipients at $(local_domains) should be
     # delivered to local mailboxes directly.
     destination $(local_domains) {
-        deliver_to local_mailboxes
+        deliver_to &local_mailboxes
     }
 
     # Remaining recipients are enqueued for remote delivery.
     default_destination {
-        deliver_to remote_queue
+        deliver_to &remote_queue
     }
 }
 
@@ -108,28 +108,25 @@ queue remote_queue {
     max_parallelism 16
 
     # Send messages to remote MTA discovered using DNS MX records.
-    target remote
+    target remote {
+        # Use MTA-STS policies and DNSSEC-signed zones to authenticate MX
+        # records before use. This is important to keep TLS secure.
+        authenticate_mx mtasts dnssec
+    }
 
     # This is how bounce messages (aka DSNs) will be routed.
     # The syntax is same as smtp/submission directives.
     bounce {
         destination $(local_domains) {
-            deliver_to local_mailboxes
+            deliver_to &local_mailboxes
         }
         default_destination {
             reject 550 5.0.0 "Refusing to send DSNs to non-local addresses"
         }
     }
 }
 
-# Configuration for remote delivery module.
-remote {
-	# Use MTA-STS policies and DNSSEC-signed zones to authenticate MX
-	# records before use. This is important to keep TLS secure.
-	authenticate_mx mtasts dnssec
-}
-
 imap tls://0.0.0.0:993 {
-    auth local_authdb
-    storage local_mailboxes
+    auth &local_authdb
+    storage &local_mailboxes
 }

man/maddy-config.5.scd

@@ -182,43 +182,6 @@ Also note that the following is not valid, unlike Duration values syntax:
 32M5K
 ```
 
-## "Inline" configuration blocks
-
-In most cases where you are supposed to specify configuration block name, you
-can instead write module name and include configuration block itself.
-
-Like that:
-```
-something {
-    auth sql {
-        driver sqlite3
-        dsn auth.db
-    }
-}
-```
-instead of
-```
-sql thing_name {
-    driver sqlite3
-    dsn auth.db
-}
-
-something {
-    auth thing_name
-}
-```
-
-In that syntax, arguments specified after module name are not considered to be
-configuration block names and instead passed directly to the module.
-Some modules use them to allow writing more compact configuration, e.g. sql module
-allows you to specify driver and DSN in "inline arguments" this way:
-```
-something {
-    auth sql sqlite3 auth.db
-}
-```
-
-
 # ADDRESS DEFINITIONS
 
 Maddy configuration uses URL-like syntax to specify network addresses.