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

Fixes some issues related to Classes as Properties and Consts validation #826

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 0 additions & 3 deletions src/CacheVerifier.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,3 @@ export class CacheVerifier {
return token === this.currentToken;
}
}


export type CacheVerifierProvider = () => CacheVerifier;
2 changes: 0 additions & 2 deletions src/Program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -689,9 +689,7 @@ export class Program {
this.logger.time(LogLevel.info, ['Validate all scopes'], () => {
for (let scopeName in this.scopes) {
let scope = this.scopes[scopeName];
scope.linkSymbolTable();
Copy link
Member

Choose a reason for hiding this comment

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

Why did you delete these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was linking symbol tables twice —- it links in validate() as well. Haha

scope.validate();
scope.unlinkSymbolTable();
}
});

Expand Down
70 changes: 69 additions & 1 deletion src/Scope.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { DiagnosticMessages } from './DiagnosticMessages';
import { Program } from './Program';
import { ParseMode } from './parser/Parser';
import PluginInterface from './PluginInterface';
import { expectDiagnostics, expectTypeToBe, expectZeroDiagnostics, trim } from './testHelpers.spec';
import { expectDiagnostics, expectDiagnosticsIncludes, expectTypeToBe, expectZeroDiagnostics, trim } from './testHelpers.spec';
import { Logger } from './Logger';
import type { BrsFile } from './files/BrsFile';
import type { FunctionStatement, NamespaceStatement } from './parser/Statement';
Expand Down Expand Up @@ -2531,5 +2531,73 @@ describe('Scope', () => {
});

});

describe('const values', () => {
it('should allow const values to be composed of other const values from namespaces', () => {
program.setFile('source/constants.bs', `
const pi = alpha.beta.pi
const two = alpha.gamma.two
const twoPi = two * pi
`);
program.setFile('source/ns.bs', `
namespace alpha.beta
const pi = 3.14
end namespace

namespace alpha.gamma
const two = 2
end namespace
`);
program.validate();
expectZeroDiagnostics(program);
});

it('should show an error when an invalid value is references', () => {
program.setFile('source/constants.bs', `
const pi = alpha.beta.pi
const two = alpha.gamma.two
const twoPi = two * pi
`);
program.setFile('source/ns.bs', `
namespace alpha.beta
const pi = 3.14
end namespace

namespace alpha.gamma
const three = 3
end namespace
`);
program.validate();
expectDiagnosticsIncludes(program, [
DiagnosticMessages.cannotFindName('two', 'alpha.gamma.two').message
]);
});
});

it('should be able to reference multiple properties of a class that is itself a property', () => {
program.setFile('source/main.bs', `
sub process(dataObj as alpha.media.MediaObject)
stream = dataObj.stream
url = stream.url
isLive = stream.live
end sub
`);
program.setFile('source/media.bs', `
namespace alpha.media
class MediaObject
stream as MediaStream
end class
end namespace

namespace alpha.media
class MediaStream
url as string
live as boolean
end class
end namespace
`);
program.validate();
expectZeroDiagnostics(program);
});
});
});
13 changes: 13 additions & 0 deletions src/SymbolTable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,13 @@ export class SymbolTable implements SymbolTypeGetter {
return resolvedType;
}

setSymbolTypeCache(name: string, resolvedType: BscType, options: GetSymbolTypeOptions) {
if (resolvedType) {
this.setCachedType(name, resolvedType, options);
}
return resolvedType;
}


/**
* Adds all the symbols from another table to this one
Expand Down Expand Up @@ -277,6 +284,11 @@ export class SymbolTable implements SymbolTypeGetter {
// no cache verifier
return;
}
let existingCachedValue = this.typeCache[options.flags]?.get(name.toLowerCase());
if (isReferenceType(type) && !isReferenceType(existingCachedValue)) {
// No need to overwrite a non-referenceType with a referenceType
return;
}
return this.typeCache[options.flags]?.set(name.toLowerCase(), type);
}

Expand Down Expand Up @@ -310,6 +322,7 @@ export interface BscSymbol {

export interface SymbolTypeGetter {
getSymbolType(name: string, options: GetSymbolTypeOptions): BscType;
setCachedType(name: string, resolvedType: BscType, options: GetSymbolTypeOptions);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/bscPlugin/validation/BrsFileValidator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ export class BrsFileValidator {
},
ConstStatement: (node) => {
this.validateDeclarationLocations(node, 'const', () => util.createBoundingRange(node.tokens.const, node.tokens.name));
const nodeType = this.getTypeFromNode(node, { flags: SymbolTypeFlag.typetime });
const nodeType = this.getTypeFromNode(node, { flags: SymbolTypeFlag.runtime });
node.parent.getSymbolTable().addSymbol(node.tokens.name.text, node.tokens.name.range, nodeType, SymbolTypeFlag.runtime);
},
CatchStatement: (node) => {
Expand Down
42 changes: 29 additions & 13 deletions src/types/ReferenceType.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import type { GetTypeOptions } from '../interfaces';
import type { CacheVerifierProvider } from '../CacheVerifier';
import type { GetSymbolTypeOptions, SymbolTypeGetterProvider } from '../SymbolTable';
import type { SymbolTypeFlag } from '../SymbolTable';
import { isDynamicType, isReferenceType } from '../astUtils/reflection';
Expand All @@ -20,7 +19,7 @@ export class ReferenceType extends BscType {
* @param flags is this type available at typetime, runtime, etc.
* @param tableProvider function that returns a SymbolTable that we use for the lookup.
*/
constructor(public memberKey: string, public fullName, public flags: SymbolTypeFlag, private tableProvider: SymbolTypeGetterProvider, private cacheVerifierProvider?: CacheVerifierProvider) {
constructor(public memberKey: string, public fullName, public flags: SymbolTypeFlag, private tableProvider: SymbolTypeGetterProvider) {
super(memberKey);
// eslint-disable-next-line no-constructor-return
return new Proxy(this, {
Expand Down Expand Up @@ -92,7 +91,7 @@ export class ReferenceType extends BscType {
return memberTypeReference;

}
memberTypeReference = new ReferenceType(memberName, this.makeMemberFullName(memberName), options.flags, this.futureMemberTableProvider, this.cacheVerifierProvider);
memberTypeReference = new ReferenceType(memberName, this.makeMemberFullName(memberName), options.flags, this.futureMemberTableProvider);
this.memberTypeReferences.set(refLookUp, memberTypeReference);
return memberTypeReference;
};
Expand Down Expand Up @@ -168,17 +167,25 @@ export class ReferenceType extends BscType {
return;
}
if (isReferenceType(resolvedType)) {
if (this.referenceChain.has(resolvedType)) {
// this is a circular reference
this.circRefCount++;
}
if (this.circRefCount > 1) {
//It is possible that we could properly resolve the case that one reference points to itself
//see test: '[Scope][symbolTable lookups with enhanced typing][finds correct class field type with default value enums are used]
return;
// If this is a referenceType, keep digging down until we have a non reference Type.
while (isReferenceType(resolvedType)) {
if (this.referenceChain.has(resolvedType)) {
// this is a circular reference
this.circRefCount++;
}
if (this.circRefCount > 1) {
//It is possible that we could properly resolve the case that one reference points to itself
//see test: '[Scope][symbolTable lookups with enhanced typing][finds correct class field type with default value enums are used]
return;
}
this.referenceChain.add(resolvedType);
resolvedType = (resolvedType as any).getTarget();

}
this.referenceChain.add(resolvedType);
} else {
this.tableProvider().setCachedType(this.memberKey, resolvedType, { flags: this.flags });
}

if (!isReferenceType(resolvedType)) {
this.circRefCount = 0;
this.referenceChain.clear();
}
Expand Down Expand Up @@ -207,6 +214,12 @@ export class ReferenceType extends BscType {
if (resolvedType) {
return resolvedType.getMemberType(innerName, innerOptions);
}
},
setCachedType: (innerName: string, innerResolvedType: BscType, options: GetSymbolTypeOptions) => {
const resolvedType = this.resolve();
if (resolvedType) {
resolvedType.memberTable.setCachedType(innerName, innerResolvedType, options);
}
}
};
};
Expand Down Expand Up @@ -245,6 +258,9 @@ export class TypePropertyReferenceType extends BscType {
return {
getSymbolType: (innerName: string, innerOptions: GetTypeOptions) => {
return this.outerType?.[this.propertyName]?.getMemberType(innerName, innerOptions);
},
setCachedType: (innerName: string, innerType: BscType, innerOptions: GetTypeOptions) => {
return this.outerType?.[this.propertyName]?.memberTable.setCachedType(innerName, innerType, innerOptions);
}
};
});
Expand Down
3 changes: 3 additions & 0 deletions src/types/UnionType.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ export class UnionType extends BscType {
return {
getSymbolType: (innerName: string, innerOptions: GetTypeOptions) => {
return getUniqueType(findTypeUnion(this.getMemberTypeFromInnerTypes(name, options)), unionTypeFactory);
},
setCachedType: (innerName: string, innerType: BscType, innerOptions: GetTypeOptions) => {
// TODO: is this even cachable? This is a NO-OP for now, and it shouldn't hurt anything
}
};
});
Expand Down