Fix several problems with symbol protection.

[Hackerpilot]

There were several problems with protection being set on symbols. An example of this is dlang-community/DCD#620.

This adds a few extra asserts, and prevents a lot of cases of symbols being created without any protection being set.

[WebFreak001]

this absolutely needs a lot of tests before I would approve this

[WebFreak001]

I think a default value is a bad idea. I would revert this definition, make it deprecated and add another overload with non-optional protection argument. This way we don't get issues with people assuming no protection and also simplify a lot of caller code which currently manually sets acSymbol.protection which isn't covered by the PR yet.

[Hackerpilot]

A few points to keep in mind:

• I think that this default is a better one than the previous default of tok!"", which is much less valid than public. public is the default for class members, and I'm pretty sure that it's the default for other things as well, but it's not actually written down in the language spec.
• allocateSemanticSymbol and pushSymbol are both private, so we're not really breaking an API.

I do plan on doing another pass over this code to make more use of this extra argument. In general this module needs a lot of cleanup and documentation.

[WebFreak001]

this protection argument isn't used

also ditto from previous comment

[Hackerpilot]

Will fix.

[WebFreak001]

let's try to make this file 100% covered with tests

[WebFreak001]

I made the following addition in tests.d for a small testing solution:

/**
 * Parses `source`, caches its symbols and compares the the cache content
 * with `expected`.
 *
 * Params:
 *      source = The source code to test.
 *      expected = An array of [variable name, protection]
 */
void expectSymbolsAndProtection(const string source, const string[2][] expected,
	string file = __FILE_FULL_PATH__, size_t line = __LINE__)
{
	import core.exception : AssertError;
	import std.array : appender;
	import std.exception : enforce;

	ModuleCache mcache = createTestModuleCache();
	auto pair = generateAutocompleteTrees(source, mcache);
	scope(exit) pair.destroy();

	string[2][] actual;

	void visit(DSymbol* symbol)
	{
		// filter no protection
		// keywords (.init, .min, .max)
		// class __vptr, __monitor, etc
		// remove empty symbol names (root symbol)
		if (symbol.protection
			&& symbol.kind != CompletionKind.keyword
			&& !symbol.name.data.startsWith("__")
			&& symbol.name.data != "classinfo"
			&& symbol.name.data.length)
		{
			actual ~= [symbol.name.data, str(symbol.protection)];
		}

		foreach(DSymbol* ss; (*symbol)[])
			visit(ss);
	}

	visit(pair.symbol);

	enforce!AssertError(actual == expected,
		"expected symbols\n[\n\t%(%s\n\t%)\n] but got\n[\n\t%(%s\n\t%)\n]".format(expected, actual),
		file, line);
}

with a test

@("exhaustive protection tests")
unittest
{
	q{
		import object;
		class DefaultFoo {}
		public class PublicFoo {}
		private struct PrivateStruct
		{
			T memberMethod(T, alias aliasArg, int intArg, VA...)(short runtimeArg, VA vaarg)
			{
				return T.init + runtimeArg * vaarg;
			}
		}
		package { enum PackageEnum { enumValue } }
		int publicInt;
		private int privateInt;
		void publicMain() {
			int localVariable;
		}
	}.expectSymbolsAndProtection([
		["DefaultFoo", "public"]
		["PublicFoo", "public"]
		["PrivateStruct", "private"] // <-- this is failing
		["*constructor*", "public"]
		["memberMethod", "public"]
		["T", "public"] // should these symbols have protection?
		["aliasArg", "public"] // ditto
		["intArg", "public"] // ditto
		["VA", "public"] // ditto
		["runtimeArg", "public"] // ditto
		["vaarg", "public"] // ditto
		["PackageEnum", "package"] // <-- this is failing
		["enumValue", "public"]
		["publicInt", "public"]
		["privateInt", "private"]
		["publicMain", "public"]
		["localVariable", "public"], // should this symbol have protection?
	]);
}

[Hackerpilot]

I think that I finally reduced a test case for the root cause of the original DCD issue:

struct Indexed
{
    static if (true)
    {
        unittest
        {
        }
    }
private:
}

int indexed()
{
    return 0;
}

This combination of static if, unittest, and private: causes indexed to be marked private.

[Hackerpilot]

All right. I found it. The problem is that SimpleParser.parseUnittest() always returns null. A lot of the parser code assumes that returning null is an error condition and so the error recovery kicks in and the parser jumps ahead and assumes that when it hits the closing braces of the static if, that it's done with the struct declaration. It then puts the private: at module scope and then errors again when it hits the closing brace on the next line.

[Hackerpilot]

I suspect that SimpleParser.parseMissingFunctionBody is wrong as well.

Edit: It is

struct Indexed
{
    static if (true)
    {
        void doStuff() do {}
    }
private:
}
int indexed;

This causes indexed to be marked private as well.

[Hackerpilot]

I still do want to do this rework of the symbol creation, but the actual fix for the the DCD issue will come in a separate pull request that fixes the parser contained in dsymbol/conversion/package.d

[WebFreak001]

moved to dlang-community/DCD#679