To add imports, the program needs to be able to distinguish between
module identifiers and local enum identifiers. Enums may be declared in
other source files within the module, so knowledge of the entire module
is required to make this distinction.

Signed-off-by: Sebastian <sebastian@sebsite.pw>
---
 main.ha | 94 ++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 77 insertions(+), 17 deletions(-)

diff --git a/main.ha b/main.ha
index 22e734b..c70750c 100644
--- a/main.ha
+++ b/main.ha
@@ -14,34 +14,94 @@
 // along with this program. If not, see http://www.gnu.org/licenses/.
 
 use fmt;
+use getopt;
 use hare::ast;
 use hare::lex;
+use hare::module;
 use hare::parse;
+use hare::unparse;
+use io;
 use os;
-use strings;
 
 export fn main() void = {
-	const lexer = lex::init(os::stdin, "<stdin>", lex::flags::NONE);
+	const help: []getopt::help = [
+		"Hare import manager",
+		('T', "tags...", "set tags"),
+		"[<path>]"
+	];
+	const cmd = getopt::parse(os::args, help...);
+	defer getopt::finish(&cmd);
 
-	const subunit = match (parse::subunit(&lexer)) {
-	case let s: ast::subunit =>
-		yield s;
-	case let e: parse::error =>
-		fmt::fatalf(parse::strerror(e));
+	let tags: []module::tag = [];
+	defer module::tags_free(tags);
+	for (let i = 0z; i < len(cmd.opts); i += 1) {
+		const opt = cmd.opts[i];
+		switch (opt.0) {
+		case 'T' =>
+			if (len(tags) > 0) {
+				getopt::printusage(os::stderr,
+					"hareimports", help);
+				os::exit(1);
+			};
+			tags = match (module::parsetags(opt.1)) {
+			case void =>
+				fmt::fatal("Invalid tag set");
+			case let t: []module::tag =>
+				yield t;
+			};
+		case => abort();
+		};
+	};
+
+	const path = switch (len(cmd.args)) {
+	case 0 =>
+		yield ".";
+	case 1 =>
+		yield cmd.args[0];
+	case =>
+		getopt::printusage(os::stderr, "hareimports", help);
+		os::exit(1);
 	};
-	defer ast::subunit_finish(subunit);
 
-	for (let i = 0z; i < len(subunit.imports); i += 1) {
-		const m = &subunit.imports[i];
+	const mctx = module::context_init(tags, [], "");
+	defer module::context_finish(&mctx);
+
+	const ver = match (module::scan(&mctx, path)) {
+	case let v: module::version =>
+		yield v;
+	case let e: module::error =>
+		fmt::fatal(module::strerror(e));
+	};
+
+	for (let i = 0z; i < len(ver.inputs); i += 1) {
+		const input = ver.inputs[i];
+		if (input.ft != module::filetype::HARE) {
+			continue;
+		};
+		const f = os::open(input.path)!;
+		defer io::close(f)!;
+
+		const lexer = lex::init(f, "<stdin>");
+		const subunit = match (parse::subunit(&lexer)) {
+		case let s: ast::subunit =>
+			yield s;
+		case let e: parse::error =>
+			fmt::fatal(parse::strerror(e));
+		};
+		defer ast::subunit_finish(subunit);
 
-		const joined_ident = strings::join("::", m.ident...);
-		defer free(joined_ident);
+		fmt::printfln("{}:", input.path)!;
+		for (let i = 0z; i < len(subunit.imports); i += 1) {
+			const m = &subunit.imports[i];
 
-		switch (m.mode) {
-		case ast::import_mode::ALIAS =>
-			fmt::printfln("{} (= {})", m.alias, joined_ident)!;
-		case =>
-			fmt::println(joined_ident)!;
+			const s = unparse::identstr(m.ident);
+			defer free(s);
+			switch (m.mode) {
+			case ast::import_mode::ALIAS =>
+				fmt::printfln("{} (= {})", m.alias, s)!;
+			case =>
+				fmt::println(s)!;
+			};
 		};
 	};
 };
-- 
2.35.1

Thanks for the substantial patch! I've spent a bit of time looking it
over as well the relevant documentation so that I can understand it as
best I can. Overall I think this looks good, howeever I have a few
questions and nitpicks:

On Sun May 22, 2022 at 7:10 PM EDT, Sebastian wrote:
> To add imports, the program needs to be able to distinguish between
> module identifiers and local enum identifiers. Enums may be declared in
> other source files within the module, so knowledge of the entire module
> is required to make this distinction.

I appreciate your forward thinking!

> Signed-off-by: Sebastian <sebastian@sebsite.pw>
> ---
>  main.ha | 94 ++++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 77 insertions(+), 17 deletions(-)
>
> diff --git a/main.ha b/main.ha
> index 22e734b..c70750c 100644
> --- a/main.ha
> +++ b/main.ha
> @@ -14,34 +14,94 @@
>  // along with this program. If not, see http://www.gnu.org/licenses/.
>  
>  use fmt;
> +use getopt;
>  use hare::ast;
>  use hare::lex;
> +use hare::module;
>  use hare::parse;
> +use hare::unparse;
> +use io;
>  use os;
> -use strings;
>  
>  export fn main() void = {
> -	const lexer = lex::init(os::stdin, "<stdin>", lex::flags::NONE);
> +	const help: []getopt::help = [
> +		"Hare import manager",
> +		('T', "tags...", "set tags"),
> +		"[<path>]"
> +	];

My own instinct for how I would add this is to provide each tag with its
own -T, but this looks to be the way tags are provided to `hare build`
so I will assume that it is good and correct.

> +	const cmd = getopt::parse(os::args, help...);
> +	defer getopt::finish(&cmd);
>  
> -	const subunit = match (parse::subunit(&lexer)) {
> -	case let s: ast::subunit =>
> -		yield s;
> -	case let e: parse::error =>
> -		fmt::fatalf(parse::strerror(e));
> +	let tags: []module::tag = [];
> +	defer module::tags_free(tags);
> +	for (let i = 0z; i < len(cmd.opts); i += 1) {
> +		const opt = cmd.opts[i];
> +		switch (opt.0) {
> +		case 'T' =>
> +			if (len(tags) > 0) {
> +				getopt::printusage(os::stderr,
> +					"hareimports", help);
> +				os::exit(1);
> +			};
> +			tags = match (module::parsetags(opt.1)) {
> +			case void =>
> +				fmt::fatal("Invalid tag set");
> +			case let t: []module::tag =>
> +				yield t;
> +			};
> +		case => abort();

I would appreciate it if you added a short string to this abort() call
explaining why this case is impossible, in the interests of friendlier
error messages.

> +		};
> +	};

Forgive my lack of experience, but I do not understand why a user would
want or need to provide build tags to this program. May you please
explain this to me? I would also appreciate if you added a comment 
briefly explaining the purpose of this passage in the context of the
program.

> +
> +	const path = switch (len(cmd.args)) {
> +	case 0 =>
> +		yield ".";
> +	case 1 =>
> +		yield cmd.args[0];
> +	case =>
> +		getopt::printusage(os::stderr, "hareimports", help);
> +		os::exit(1);
>  	};
> -	defer ast::subunit_finish(subunit);
>  
> -	for (let i = 0z; i < len(subunit.imports); i += 1) {
> -		const m = &subunit.imports[i];
> +	const mctx = module::context_init(tags, [], "");
> +	defer module::context_finish(&mctx);
> +
> +	const ver = match (module::scan(&mctx, path)) {
> +	case let v: module::version =>
> +		yield v;
> +	case let e: module::error =>
> +		fmt::fatal(module::strerror(e));
> +	};
> +
> +	for (let i = 0z; i < len(ver.inputs); i += 1) {
> +		const input = ver.inputs[i];
> +		if (input.ft != module::filetype::HARE) {
> +			continue;
> +		};
> +		const f = os::open(input.path)!;
> +		defer io::close(f)!;
> +
> +		const lexer = lex::init(f, "<stdin>");

We should provide the lexer with an actual path now that we are not
reading from stdin.

> +		const subunit = match (parse::subunit(&lexer)) {
> +		case let s: ast::subunit =>
> +			yield s;
> +		case let e: parse::error =>
> +			fmt::fatal(parse::strerror(e));
> +		};
> +		defer ast::subunit_finish(subunit);
>  
> -		const joined_ident = strings::join("::", m.ident...);
> -		defer free(joined_ident);
> +		fmt::printfln("{}:", input.path)!;
> +		for (let i = 0z; i < len(subunit.imports); i += 1) {
> +			const m = &subunit.imports[i];
>  
> -		switch (m.mode) {
> -		case ast::import_mode::ALIAS =>
> -			fmt::printfln("{} (= {})", m.alias, joined_ident)!;
> -		case =>
> -			fmt::println(joined_ident)!;
> +			const s = unparse::identstr(m.ident);
> +			defer free(s);
> +			switch (m.mode) {
> +			case ast::import_mode::ALIAS =>
> +				fmt::printfln("{} (= {})", m.alias, s)!;
> +			case =>
> +				fmt::println(s)!;
> +			};
>  		};
>  	};
>  };
> -- 
> 2.35.1

Overall I think this is a good expansion to the skeleton of the program.
I look forward to your response/v2.

Thanks again,
Sebastian L.

May 22, 2022 21:12:30 Sebastian LaVine <mail@smlavine.com>:
> My own instinct for how I would add this is to provide each tag with 
> its
> own -T, but this looks to be the way tags are provided to `hare build`
> so I will assume that it is good and correct.

hare build actually accepts both. See addtags and deltags in 
cmd/hare/subcmds.ha. I didn't include that in this patch, but I can add 
that in v2 if it's desired.

> Forgive my lack of experience, but I do not understand why a user would
> want or need to provide build tags to this program. May you please
> explain this to me?

Tags are used to conditionally include source files in a module. So, a 
type may be defined for one tag set, but not for another. This way, 
hareimports knows whether an identifier is a module that should be 
imported, or a local enum defined in a specific tag set.

Though, now that I think about it, maybe it should just search through 
all files, ignoring tags. Hm, not sure.

> We should provide the lexer with an actual path now that we are not
> reading from stdin.

Ah, whoops!

I'll send a v2 once the questions here are answered. Thanks!
- Sebastian

On Mon May 23, 2022 at 8:36 AM EDT, Sebastian wrote:
> May 22, 2022 21:12:30 Sebastian LaVine <mail@smlavine.com>:
> > My own instinct for how I would add this is to provide each tag with 
> > its
> > own -T, but this looks to be the way tags are provided to `hare build`
> > so I will assume that it is good and correct.
>
> hare build actually accepts both. See addtags and deltags in 
> cmd/hare/subcmds.ha. I didn't include that in this patch, but I can add 
> that in v2 if it's desired.

I'd prefer if you added that functionality in a separate patch rather
than in this v2. Consistency with hare build would be preferred but it's
not a priority for now.

> > Forgive my lack of experience, but I do not understand why a user would
> > want or need to provide build tags to this program. May you please
> > explain this to me?
>
> Tags are used to conditionally include source files in a module. So, a 
> type may be defined for one tag set, but not for another. This way, 
> hareimports knows whether an identifier is a module that should be 
> imported, or a local enum defined in a specific tag set.

I think I understand what you mean. I set up a little test case here:

main.ha:
```
use fmt;
use strings;

export fn main() void = {
        let s = strings::dup("Hello");
        defer free(s);

        fmt::println(s)!;
};
```

colors+blue.ha:
```
type strings = enum uint {
        dup,
};
```

In this case, the program does not compile if +blue is set. And I can
imagine that there are cases of silent overlap, too. So I can see this
feature being useful, to settle this ambiguity in hypothetical cases.

The thing I'm thinking about too though is that I imagine that this
program will be configured to run automatically on file write, like I
currently have with Go programs via [vim-go][0]. In such an environment,
I wonder how useful -T would really be.

[0]: https://github.com/fatih/vim-go

I am leaning slightly toward adding the flag. But I leave it to you to
decide whether or not to keep it in your v2, and I'll trust your
judgement.

If you do keep it in, please add a comment or two explaining briefly why
it exists and could be useful. And if you do take it out, please leave
the rest of the getopt boilerplate that you added; I plan on adding some
other options anyway.

I appreciate your interest in this project in its early stages. I look
forward to your response!

Oh, and if anyone else is on this list, I'd appreciate your feedback
too.

-- 
Sebastian LaVine | https://smlavine.com

Sorry I'm taking a while with this; I've been busy and haven't gotten the 
chance to finish this up. I should have a patch ready by either today or 
tomorrow.
- Sebastian

Sounds good! No worries.