Skip to content

Fix several problems with symbol protection. #153

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.

Sign up for GitHub

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

Closed
Hackerpilot wants to merge 2 commits into from
Closed

Fix several problems with symbol protection. #153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ca7a59
Fix several problems with symbol protection that were pointed out in …
Hackerpilot Jul 23, 2020
e70c422
Fix a bug with the default visibility of imported symbols
Hackerpilot Jul 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/dsymbol/builtin/symbols.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,15 +294,21 @@ private HashSet!(DSymbol*) symbolsMadeHere;

private DSymbol* makeSymbol(string s, CompletionKind kind, DSymbol* type = null)
{
import dparse.lexer : tok;

auto sym = rba.make!DSymbol(istring(s), kind, type);
sym.ownType = false;
sym.protection = tok!"public";
symbolsMadeHere.insert(sym);
return sym;
}
private DSymbol* makeSymbol(istring s, CompletionKind kind, DSymbol* type = null)
{
import dparse.lexer : tok;

auto sym = rba.make!DSymbol(s, kind, type);
sym.ownType = false;
sym.protection = tok!"public";
symbolsMadeHere.insert(sym);
return sym;
}
34 changes: 22 additions & 12 deletions src/dsymbol/conversion/first.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,8 @@ final class FirstPass : ASTVisitor
{
assert(dec);
pushSymbol(dec.name.text, CompletionKind.functionName, symbolFile,
dec.name.index, dec.returnType);
dec.name.index, dec.returnType, protection.current);
scope (exit) popSymbol();
currentSymbol.acSymbol.protection = protection.current;
currentSymbol.acSymbol.doc = makeDocumentation(dec.comment);

istring lastComment = this.lastComment;
Expand Down Expand Up @@ -387,6 +386,7 @@ final class FirstPass : ASTVisitor
auto objectImport = allocateSemanticSymbol(IMPORT_SYMBOL_NAME,
CompletionKind.importSymbol, objectLocation);
objectImport.acSymbol.skipOver = true;
objectImport.acSymbol.protection = protection.currentForImport;
currentSymbol.addChild(objectImport, true);
currentScope.addSymbol(objectImport.acSymbol, false);
}
Expand Down Expand Up @@ -453,6 +453,7 @@ final class FirstPass : ASTVisitor
thisSymbol.symbolFile = symbolFile;
thisSymbol.type = currentSymbol.acSymbol;
thisSymbol.ownType = false;
thisSymbol.protection = tok!"private";
currentScope.addSymbol(thisSymbol, false);

foreach (dec; structBody.declarations)
Expand Down Expand Up @@ -484,6 +485,7 @@ final class FirstPass : ASTVisitor
SemanticSymbol* importSymbol = allocateSemanticSymbol(IMPORT_SYMBOL_NAME,
CompletionKind.importSymbol, modulePath);
importSymbol.acSymbol.skipOver = protection.currentForImport != tok!"public";
importSymbol.acSymbol.protection = protection.currentForImport;
if (single.rename == tok!"")
{
size_t i = 0;
Expand All @@ -501,6 +503,8 @@ final class FirstPass : ASTVisitor
if (s.length == 0)
{
currentImportSymbol = symbolAllocator.make!DSymbol(ip, kind);
currentImportSymbol.protection = protection.currentForImport;
currentImportSymbol.skipOver = protection.currentForImport != tok!"public";
currentScope.addSymbol(currentImportSymbol, true);
if (last)
{
Expand All @@ -518,6 +522,8 @@ final class FirstPass : ASTVisitor
if (s.length == 0)
{
auto sym = symbolAllocator.make!DSymbol(ip, kind);
sym.protection = protection.currentForImport;
sym.skipOver = protection.currentForImport != tok!"public";
currentImportSymbol.addChild(sym, true);
currentImportSymbol = sym;
if (last)
Expand All @@ -540,6 +546,7 @@ final class FirstPass : ASTVisitor
SemanticSymbol* renameSymbol = allocateSemanticSymbol(
internString(single.rename.text), CompletionKind.aliasName,
modulePath);
renameSymbol.acSymbol.protection = protection.currentForImport;
renameSymbol.acSymbol.skipOver = protection.currentForImport != tok!"public";
renameSymbol.acSymbol.type = importSymbol.acSymbol;
renameSymbol.acSymbol.ownType = true;
Expand All @@ -557,7 +564,7 @@ final class FirstPass : ASTVisitor
istring modulePath = cache.resolveImportLocation(chain);
if (modulePath is null)
{
warning("Could not resolve location of module '", chain, "'");
warning("Could not resolve location of module '", chain.data, "'");
return;
}

Expand Down Expand Up @@ -585,6 +592,7 @@ final class FirstPass : ASTVisitor
importSymbol.acSymbol.qualifier = SymbolQualifier.selectiveImport;
importSymbol.typeLookups.insert(lookup);
importSymbol.acSymbol.skipOver = protection.currentForImport != tok!"public";
importSymbol.acSymbol.protection = protection.currentForImport;
currentSymbol.addChild(importSymbol, true);
currentScope.addSymbol(importSymbol.acSymbol, false);
}
Expand Down Expand Up @@ -848,7 +856,7 @@ private:
}

void pushSymbol(string name, CompletionKind kind, istring symbolFile,
size_t location = 0, const Type type = null)
size_t location = 0, const Type type = null, const IdType protection = tok!"public")
Copy link
Copy Markdown
Member

@WebFreak001 WebFreak001 Jul 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this protection argument isn't used

also ditto from previous comment

Copy link
Copy Markdown
Collaborator Author

@Hackerpilot Hackerpilot Jul 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix.

{
SemanticSymbol* symbol = allocateSemanticSymbol(name, kind, symbolFile,
location);
Expand Down Expand Up @@ -884,14 +892,13 @@ private:
dec.accept(this);
return;
}
pushSymbol(dec.name.text, kind, symbolFile, dec.name.index);
pushSymbol(dec.name.text, kind, symbolFile, dec.name.index, null, protection.current);
scope(exit) popSymbol();

if (kind == CompletionKind.className)
currentSymbol.acSymbol.addChildren(classSymbols[], false);
else
currentSymbol.acSymbol.addChildren(aggregateSymbols[], false);
currentSymbol.acSymbol.protection = protection.current;
currentSymbol.acSymbol.doc = makeDocumentation(dec.comment);

istring lastComment = this.lastComment;
Expand Down Expand Up @@ -1106,7 +1113,7 @@ private:
}

SemanticSymbol* allocateSemanticSymbol(string name, CompletionKind kind,
istring symbolFile, size_t location = 0)
istring symbolFile, size_t location = 0, IdType protection = tok!"public")
Copy link
Copy Markdown
Member

@WebFreak001 WebFreak001 Jul 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

@Hackerpilot Hackerpilot Jul 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

in
{
assert (symbolAllocator !is null);
Expand All @@ -1116,6 +1123,7 @@ private:
DSymbol* acSymbol = make!DSymbol(symbolAllocator, istring(name), kind);
acSymbol.location = location;
acSymbol.symbolFile = symbolFile;
acSymbol.protection = protection;
symbolsAllocated++;
return semanticAllocator.make!SemanticSymbol(acSymbol);
}
Expand Down Expand Up @@ -1235,17 +1243,19 @@ struct ProtectionStack

IdType currentForImport() const
{
return stack.empty ? tok!"default" : current();
// Imports are private unless specified otherwise.
return stack.empty ? tok!"private" : current();
}

IdType current() const
out(t) { assert(isProtection(t), str(t)); }
do
{
import std.algorithm.iteration : filter;
import std.range : choose, only;

IdType retVal;
foreach (t; choose(stack.empty, only(tok!"public"), stack[]).filter!(
a => a != tok!"{" && a != tok!":"))
IdType retVal = tok!"public";
foreach (t; stack[].filter!(a => a != tok!"{" && a != tok!":"))
retVal = cast(IdType) t;
return retVal;
}
Expand Down Expand Up @@ -1274,7 +1284,7 @@ struct ProtectionStack

void beginLocal(const IdType t)
{
assert (t != tok!"", "DERP!");
assert(isProtection(t), str(t));
stack.insertBack(t);
}

Expand Down
3 changes: 3 additions & 0 deletions src/dsymbol/conversion/second.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,7 @@ void resolveInheritance(DSymbol* symbol, ref UnrolledList!(TypeLookup*, Mallocat

DSymbol* imp = cache.symbolAllocator.make!DSymbol(IMPORT_SYMBOL_NAME,
CompletionKind.importSymbol, baseClass);
imp.protection = tok!"public";
symbol.addChild(imp, true);
symbolScope.addSymbol(imp, false);
if (baseClass.kind == CompletionKind.className)
Expand All @@ -360,6 +361,7 @@ void resolveAliasThis(DSymbol* symbol,
continue;
DSymbol* s = cache.symbolAllocator.make!DSymbol(IMPORT_SYMBOL_NAME,
CompletionKind.importSymbol, parts[0].type);
s.protection = tok!"public";
symbol.addChild(s, true);
auto symbolScope = moduleScope.getScopeByCursor(s.location);
if (symbolScope !is null)
Expand Down Expand Up @@ -397,6 +399,7 @@ void resolveMixinTemplates(DSymbol* symbol,
auto i = cache.symbolAllocator.make!DSymbol(IMPORT_SYMBOL_NAME,
CompletionKind.importSymbol, currentSymbol);
i.ownType = false;
i.protection = tok!"public";
symbol.addChild(i, true);
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/dsymbol/symbol.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@ struct DSymbol
void addChild(DSymbol* symbol, bool owns)
{
assert(symbol !is null);
assert(isProtection(symbol.protection));
parts.insert(SymbolOwnership(symbol, owns));
}

Expand All @@ -307,6 +308,7 @@ struct DSymbol
foreach (symbol; symbols)
{
assert(symbol !is null);
assert(isProtection(symbol.protection));
parts.insert(SymbolOwnership(symbol, owns));
}
}
Expand All @@ -316,6 +318,7 @@ struct DSymbol
foreach (symbol; symbols)
{
assert(symbol !is null);
assert(isProtection(symbol.protection));
parts.insert(SymbolOwnership(symbol, owns));
}
}
Expand Down