Allow extending from any - #14935

Merged
Nathan Shively-Sanders (sandersn) merged 12 commits into
masterfrom
allow-extending-from-any
Apr 6, 2017
Merged

Allow extending from any#14935
Nathan Shively-Sanders (sandersn) merged 12 commits into
masterfrom
allow-extending-from-any

Conversation

@sandersn

@sandersnNathan Shively-Sanders (sandersn) commented Mar 30, 2017•
edited
Loading

Copy link
Copy Markdown
Member

Extending from any adds an index signature: [s: string]: any to both the
instance and static sides of the class.

Fixes#14301
Fixes#15040

@sandersn
Allow extending from any
f1c1969
Extending from any adds an index signature: [s: string]: any to both the
instance and static sides of the class.
@sandersn
Improve handling of any as a base type
765ac7c
Also improve how the string indexer for any-inheriting types is added.
@sandersn
Test: extending from any
650cc29
@sandersn
Update baselines
3624417
@sandersn
Re-swap mistakenly swapped lines of code
41eabbc
alfaproject
João Dias (alfaproject) reviewed Mar 31, 2017
View reviewed changes
Comment threadtests/baselines/reference/extendFromAny.js Outdated
}

let c = new C();
c.known.length; // error, 'real' has no 'length' property

@alfaprojectJoão Dias (alfaproject)Mar 31, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

real / sreal

@sandersn

Nathan Shively-Sanders (sandersn) commented Apr 5, 2017

Copy link
Copy Markdown
MemberAuthor

Yui (@yuit)Andy (Andrewkraft) (@Andy-MS) mind taking a look?

DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) reviewed Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts Outdated
return type.resolvedBaseConstructorType = unknownType;
}
if (baseConstructorType !== unknownType && baseConstructorType !== nullWideningType && !isConstructorType(baseConstructorType)) {
if (baseConstructorType !== anyType && baseConstructorType !== unknownType && baseConstructorType !== nullWideningType && !isConstructorType(baseConstructorType)) {

@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser)Apr 5, 2017•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hard for me to look up right now whether unknownType includes it, could you check for TypeFlags.Any?

@yuitYui (yuit)Apr 5, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just check unknownType and anyType both have TypeFlags.Any

@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Includes anyType, autoType, unknownType. Seems like it should work.

@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

done

yuit
Yui (yuit) reviewed Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts Outdated
function isValidBaseType(type: Type): boolean {
return type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive) && !isGenericMappedType(type) ||
return !!(type.flags & TypeFlags.Any) ||
type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive) && !isGenericMappedType(type) ||

@yuitYui (yuit)Apr 5, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should this just be type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive | TypeFlags.Any)

@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think it should work, although it's kind of a weird pun in that we rely on !isGenericMappedType(anyType) being true.

yuit
Yui (yuit) reviewed Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts Outdated
callSignatures = concatenate(callSignatures, getSignaturesOfType(instantiatedBaseType, SignatureKind.Call));
constructSignatures = concatenate(constructSignatures, getSignaturesOfType(instantiatedBaseType, SignatureKind.Construct));
stringIndexInfo = stringIndexInfo || getIndexInfoOfType(instantiatedBaseType, IndexKind.String);
stringIndexInfo = stringIndexInfo || (instantiatedBaseType === anyType ?

@yuitYui (yuit)Apr 5, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: would it be easier to read this way:

if(!stringIndexInfo){stringIndexInfo=instantiatedBaseType===anyType ? createIndexInfo(anyType,/*isReadonly*/false) : getIndexInfoOfType(instantiatedBaseType,IndexKind.String);}

@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I guess so. Done.

@ghost

Deleted user (ghost) commented Apr 5, 2017•
edited by ghost
Loading

Copy link
Copy Markdown

I wonder if anything from #12352 could be simplified now.
At least the comment about getBaseConstructorTypeOfClass could be updated.

ghost
Deleted user (ghost) reviewed Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts Outdated
baseType = getTypeFromClassOrInterfaceReference(baseTypeNode, baseConstructorType.symbol);
}
else if (baseConstructorType.flags & TypeFlags.Any) {
baseType = anyType;

@ghostDeleted user (ghost)Apr 5, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could you leave a comment saying why it is necessary to set it to anyType instead of baseConstructorType?

@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

There's no good reason; in fact it reduces test churn to return baseConstructorType for tests that extend undefined variables, which I think get unknownType instead of anyType, meaning that you get slightly more errors.

I'll change it to baseConstructorType.

@sandersn

Nathan Shively-Sanders (sandersn) commented Apr 5, 2017

Copy link
Copy Markdown
MemberAuthor

Andy (Andrewkraft) (@Andy-MS) Maybe? I don't understand the differences between using unknownSymbol (which I guess has unknownType) for a missing module vs creating a synthetic symbol. Did the synthetic symbol get created solely for error reporting?

Seems like it would be worth reverting #12352 to see how it behaves, because I'm not able to predict much about module behaviour a priori.

@ghost

Deleted user (ghost) commented Apr 5, 2017

Copy link
Copy Markdown

OK, we can try reverting #12352 later.
But please update this comment:

// This must be different than unknownSymbol because getBaseConstructorTypeOfClass won't fail for unknownSymbol.

Closed
@sandersn
Remove special symbol for untyped modules
90d5c29
Extending symbols from untyped modules is no longer an error, so #12532
didn't get us anything except slightly better quick info.
@sandersn

Nathan Shively-Sanders (sandersn) commented Apr 5, 2017

Copy link
Copy Markdown
MemberAuthor

Reverting #12352 turned out to be straightforward, so I ended up merging that branch back into this PR.

ghost
Deleted user (ghost) reviewed Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
const name = specifier.propertyName || specifier.name;
if (name.text) {
if (isUntypedOrShorthandAmbientModuleSymbol(moduleSymbol)) {
if (isShorthandAmbientModuleSymbol(moduleSymbol)) {

@ghostDeleted user (ghost)Apr 5, 2017•
edited by ghost
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I like the old name better, even if it's a bit long.

@sandersn
Improve test and comments
f635042
Test asserts that unused locals error works for untyped modules.
Comment no longer claims to check for untyped modules.
@sandersn
Nathan Shively-Sanders (sandersn) deleted the allow-extending-from-any branch April 6, 2017 16:18
@ghostghost mentioned this pull request Jul 8, 2017
Closed
@microsoftMicrosoft (microsoft) locked and limited conversation to collaborators Jun 21, 2018
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser)DanielRosenwasser left review comments
+3 more reviewers
@alfaprojectJoão Dias (alfaproject)alfaproject left review comments
@yuitYui (yuit)yuit approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sandersn@alfaproject@yuit@DanielRosenwasser@msftclas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Allow extending from any - #14935
Merged
Nathan Shively-Sanders (sandersn) merged 12 commits into
masterfrom 
allow-extending-from-any
Apr 6, 2017
Merged
Allow extending from any#14935
Nathan Shively-Sanders (sandersn) merged 12 commits into
masterfrom 
allow-extending-from-any
Conversation
@sandersn
@sandersnNathan Shively-Sanders (sandersn)
commented
Mar 30, 2017•

edited

Loading

Copy link


Copy Markdown

Member
Extending from any adds an index signature: [s: string]: any to both the

instance and static sides of the class.
Fixes#14301
Fixes#15040
@sandersn
Allow extending from any
f1c1969
Extending from any adds an index signature: [s: string]: any to both the
instance and static sides of the class.
@sandersn
Improve handling of any as a base type
765ac7c
Also improve how the string indexer for any-inheriting types is added.
@sandersn
Test: extending from any
650cc29
@sandersn
Update baselines
3624417
@sandersn
Re-swap mistakenly swapped lines of code
41eabbc
alfaproject
João Dias (alfaproject)
reviewed
Mar 31, 2017
View reviewed changes
Comment threadtests/baselines/reference/extendFromAny.js
Outdated

}

let c = new C();
c.known.length; // error, 'real' has no 'length' property
@alfaprojectJoão Dias (alfaproject)Mar 31, 2017

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

real / sreal
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Yui (@yuit)Andy (Andrewkraft) (@Andy-MS) mind taking a look?
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 return type.resolvedBaseConstructorType = unknownType;
 }
 if (baseConstructorType !== unknownType && baseConstructorType !== nullWideningType && !isConstructorType(baseConstructorType)) {
 if (baseConstructorType !== anyType && baseConstructorType !== unknownType && baseConstructorType !== nullWideningType && !isConstructorType(baseConstructorType)) {
@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser)Apr 5, 2017•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Hard for me to look up right now whether unknownType includes it, could you check for TypeFlags.Any?
@yuitYui (yuit)Apr 5, 2017

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just check unknownType and anyType both have TypeFlags.Any
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Includes anyType, autoType, unknownType. Seems like it should work.
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

done
yuit
Yui (yuit)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 function isValidBaseType(type: Type): boolean {
 return type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive) && !isGenericMappedType(type) ||
 return !!(type.flags & TypeFlags.Any) ||
 type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive) && !isGenericMappedType(type) ||
@yuitYui (yuit)Apr 5, 2017•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

should this just be type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive | TypeFlags.Any)
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I think it should work, although it's kind of a weird pun in that we rely on !isGenericMappedType(anyType) being true.
yuit
Yui (yuit)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 callSignatures = concatenate(callSignatures, getSignaturesOfType(instantiatedBaseType, SignatureKind.Call));
 constructSignatures = concatenate(constructSignatures, getSignaturesOfType(instantiatedBaseType, SignatureKind.Construct));
 stringIndexInfo = stringIndexInfo || getIndexInfoOfType(instantiatedBaseType, IndexKind.String);
 stringIndexInfo = stringIndexInfo || (instantiatedBaseType === anyType ?
@yuitYui (yuit)Apr 5, 2017

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

nit: would it be easier to read this way:
if(!stringIndexInfo){stringIndexInfo=instantiatedBaseType===anyType ? createIndexInfo(anyType,/*isReadonly*/false) : getIndexInfoOfType(instantiatedBaseType,IndexKind.String);}
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I guess so. Done.
@ghost
Deleted user (ghost)
commented
Apr 5, 2017•

edited by ghost

Loading

Copy link


Copy Markdown

I wonder if anything from #12352 could be simplified now.

At least the comment about getBaseConstructorTypeOfClass could be updated.
ghost
Deleted user (ghost)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 baseType = getTypeFromClassOrInterfaceReference(baseTypeNode, baseConstructorType.symbol);
 }
 else if (baseConstructorType.flags & TypeFlags.Any) {
 baseType = anyType;
@ghostDeleted user (ghost)Apr 5, 2017

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Could you leave a comment saying why it is necessary to set it to anyType instead of baseConstructorType?
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

There's no good reason; in fact it reduces test churn to return baseConstructorType for tests that extend undefined variables, which I think get unknownType instead of anyType, meaning that you get slightly more errors.
I'll change it to baseConstructorType.
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Andy (Andrewkraft) (@Andy-MS) Maybe? I don't understand the differences between using unknownSymbol (which I guess has unknownType) for a missing module vs creating a synthetic symbol. Did the synthetic symbol get created solely for error reporting?
Seems like it would be worth reverting #12352 to see how it behaves, because I'm not able to predict much about module behaviour a priori.
@ghost
Deleted user (ghost)
commented
Apr 5, 2017

Copy link


Copy Markdown

OK, we can try reverting #12352 later.

But please update this comment:
// This must be different than unknownSymbol because getBaseConstructorTypeOfClass won't fail for unknownSymbol.

Closed

@sandersn
Remove special symbol for untyped modules
90d5c29
Extending symbols from untyped modules is no longer an error, so #12532
didn't get us anything except slightly better quick info.
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Reverting #12352 turned out to be straightforward, so I ended up merging that branch back into this PR.
ghost
Deleted user (ghost)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
 const name = specifier.propertyName || specifier.name;
 if (name.text) {
 if (isUntypedOrShorthandAmbientModuleSymbol(moduleSymbol)) {
 if (isShorthandAmbientModuleSymbol(moduleSymbol)) {
@ghostDeleted user (ghost)Apr 5, 2017•

edited by ghost

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I like the old name better, even if it's a bit long.
@sandersn
Improve test and comments
f635042
Test asserts that unused locals error works for untyped modules.
Comment no longer claims to check for untyped modules.
@sandersn
Nathan Shively-Sanders (sandersn)
deleted the

allow-extending-from-any

branch
April 6, 2017 16:18
@ghostghost
mentioned this pull request
Jul 8, 2017

Closed

@microsoftMicrosoft (microsoft)
locked and limited conversation to collaborators
Jun 21, 2018
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser)DanielRosenwasser left review comments

+3 more reviewers

@alfaprojectJoão Dias (alfaproject)alfaproject left review comments
@yuitYui (yuit)yuit approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@sandersn@alfaproject@yuit@DanielRosenwasser@msftclas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Allow extending from any - #14935
Merged
Nathan Shively-Sanders (sandersn) merged 12 commits into
masterfrom 
allow-extending-from-any
Apr 6, 2017
Merged
Allow extending from any#14935
Nathan Shively-Sanders (sandersn) merged 12 commits into
masterfrom 
allow-extending-from-any
Conversation
@sandersn
@sandersnNathan Shively-Sanders (sandersn)
commented
Mar 30, 2017•

edited

Loading

Copy link


Copy Markdown

Member
Extending from any adds an index signature: [s: string]: any to both the

instance and static sides of the class.
Fixes#14301
Fixes#15040
@sandersn
Allow extending from any
f1c1969
Extending from any adds an index signature: [s: string]: any to both the
instance and static sides of the class.
@sandersn
Improve handling of any as a base type
765ac7c
Also improve how the string indexer for any-inheriting types is added.
@sandersn
Test: extending from any
650cc29
@sandersn
Update baselines
3624417
@sandersn
Re-swap mistakenly swapped lines of code
41eabbc
alfaproject
João Dias (alfaproject)
reviewed
Mar 31, 2017
View reviewed changes
Comment threadtests/baselines/reference/extendFromAny.js
Outdated

}

let c = new C();
c.known.length; // error, 'real' has no 'length' property
@alfaprojectJoão Dias (alfaproject)Mar 31, 2017

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

real / sreal
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Yui (@yuit)Andy (Andrewkraft) (@Andy-MS) mind taking a look?
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 return type.resolvedBaseConstructorType = unknownType;
 }
 if (baseConstructorType !== unknownType && baseConstructorType !== nullWideningType && !isConstructorType(baseConstructorType)) {
 if (baseConstructorType !== anyType && baseConstructorType !== unknownType && baseConstructorType !== nullWideningType && !isConstructorType(baseConstructorType)) {
@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser)Apr 5, 2017•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Hard for me to look up right now whether unknownType includes it, could you check for TypeFlags.Any?
@yuitYui (yuit)Apr 5, 2017

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just check unknownType and anyType both have TypeFlags.Any
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Includes anyType, autoType, unknownType. Seems like it should work.
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

done
yuit
Yui (yuit)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 function isValidBaseType(type: Type): boolean {
 return type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive) && !isGenericMappedType(type) ||
 return !!(type.flags & TypeFlags.Any) ||
 type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive) && !isGenericMappedType(type) ||
@yuitYui (yuit)Apr 5, 2017•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

should this just be type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive | TypeFlags.Any)
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I think it should work, although it's kind of a weird pun in that we rely on !isGenericMappedType(anyType) being true.
yuit
Yui (yuit)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 callSignatures = concatenate(callSignatures, getSignaturesOfType(instantiatedBaseType, SignatureKind.Call));
 constructSignatures = concatenate(constructSignatures, getSignaturesOfType(instantiatedBaseType, SignatureKind.Construct));
 stringIndexInfo = stringIndexInfo || getIndexInfoOfType(instantiatedBaseType, IndexKind.String);
 stringIndexInfo = stringIndexInfo || (instantiatedBaseType === anyType ?
@yuitYui (yuit)Apr 5, 2017

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

nit: would it be easier to read this way:
if(!stringIndexInfo){stringIndexInfo=instantiatedBaseType===anyType ? createIndexInfo(anyType,/*isReadonly*/false) : getIndexInfoOfType(instantiatedBaseType,IndexKind.String);}
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I guess so. Done.
@ghost
Deleted user (ghost)
commented
Apr 5, 2017•

edited by ghost

Loading

Copy link


Copy Markdown

I wonder if anything from #12352 could be simplified now.

At least the comment about getBaseConstructorTypeOfClass could be updated.
ghost
Deleted user (ghost)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 baseType = getTypeFromClassOrInterfaceReference(baseTypeNode, baseConstructorType.symbol);
 }
 else if (baseConstructorType.flags & TypeFlags.Any) {
 baseType = anyType;
@ghostDeleted user (ghost)Apr 5, 2017

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Could you leave a comment saying why it is necessary to set it to anyType instead of baseConstructorType?
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

There's no good reason; in fact it reduces test churn to return baseConstructorType for tests that extend undefined variables, which I think get unknownType instead of anyType, meaning that you get slightly more errors.
I'll change it to baseConstructorType.
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Andy (Andrewkraft) (@Andy-MS) Maybe? I don't understand the differences between using unknownSymbol (which I guess has unknownType) for a missing module vs creating a synthetic symbol. Did the synthetic symbol get created solely for error reporting?
Seems like it would be worth reverting #12352 to see how it behaves, because I'm not able to predict much about module behaviour a priori.
@ghost
Deleted user (ghost)
commented
Apr 5, 2017

Copy link


Copy Markdown

OK, we can try reverting #12352 later.

But please update this comment:
// This must be different than unknownSymbol because getBaseConstructorTypeOfClass won't fail for unknownSymbol.

Closed

@sandersn
Remove special symbol for untyped modules
90d5c29
Extending symbols from untyped modules is no longer an error, so #12532
didn't get us anything except slightly better quick info.
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Reverting #12352 turned out to be straightforward, so I ended up merging that branch back into this PR.
ghost
Deleted user (ghost)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
 const name = specifier.propertyName || specifier.name;
 if (name.text) {
 if (isUntypedOrShorthandAmbientModuleSymbol(moduleSymbol)) {
 if (isShorthandAmbientModuleSymbol(moduleSymbol)) {
@ghostDeleted user (ghost)Apr 5, 2017•

edited by ghost

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I like the old name better, even if it's a bit long.
@sandersn
Improve test and comments
f635042
Test asserts that unused locals error works for untyped modules.
Comment no longer claims to check for untyped modules.
@sandersn
Nathan Shively-Sanders (sandersn)
deleted the

allow-extending-from-any

branch
April 6, 2017 16:18
@ghostghost
mentioned this pull request
Jul 8, 2017

Closed

@microsoftMicrosoft (microsoft)
locked and limited conversation to collaborators
Jun 21, 2018
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser)DanielRosenwasser left review comments

+3 more reviewers

@alfaprojectJoão Dias (alfaproject)alfaproject left review comments
@yuitYui (yuit)yuit approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@sandersn@alfaproject@yuit@DanielRosenwasser@msftclas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Allow extending from any - #14935
Merged
Nathan Shively-Sanders (sandersn) merged 12 commits into
masterfrom 
allow-extending-from-any
Apr 6, 2017
Merged
Allow extending from any#14935
Nathan Shively-Sanders (sandersn) merged 12 commits into
masterfrom 
allow-extending-from-any
Conversation
@sandersn
@sandersnNathan Shively-Sanders (sandersn)
commented
Mar 30, 2017•

edited

Loading

Copy link


Copy Markdown

Member
Extending from any adds an index signature: [s: string]: any to both the

instance and static sides of the class.
Fixes#14301
Fixes#15040
@sandersn
Allow extending from any
f1c1969
Extending from any adds an index signature: [s: string]: any to both the
instance and static sides of the class.
@sandersn
Improve handling of any as a base type
765ac7c
Also improve how the string indexer for any-inheriting types is added.
@sandersn
Test: extending from any
650cc29
@sandersn
Update baselines
3624417
@sandersn
Re-swap mistakenly swapped lines of code
41eabbc
alfaproject
João Dias (alfaproject)
reviewed
Mar 31, 2017
View reviewed changes
Comment threadtests/baselines/reference/extendFromAny.js
Outdated

}

let c = new C();
c.known.length; // error, 'real' has no 'length' property
@alfaprojectJoão Dias (alfaproject)Mar 31, 2017

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

real / sreal
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Yui (@yuit)Andy (Andrewkraft) (@Andy-MS) mind taking a look?
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 return type.resolvedBaseConstructorType = unknownType;
 }
 if (baseConstructorType !== unknownType && baseConstructorType !== nullWideningType && !isConstructorType(baseConstructorType)) {
 if (baseConstructorType !== anyType && baseConstructorType !== unknownType && baseConstructorType !== nullWideningType && !isConstructorType(baseConstructorType)) {
@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser)Apr 5, 2017•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Hard for me to look up right now whether unknownType includes it, could you check for TypeFlags.Any?
@yuitYui (yuit)Apr 5, 2017

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just check unknownType and anyType both have TypeFlags.Any
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Includes anyType, autoType, unknownType. Seems like it should work.
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

done
yuit
Yui (yuit)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 function isValidBaseType(type: Type): boolean {
 return type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive) && !isGenericMappedType(type) ||
 return !!(type.flags & TypeFlags.Any) ||
 type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive) && !isGenericMappedType(type) ||
@yuitYui (yuit)Apr 5, 2017•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

should this just be type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive | TypeFlags.Any)
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I think it should work, although it's kind of a weird pun in that we rely on !isGenericMappedType(anyType) being true.
yuit
Yui (yuit)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 callSignatures = concatenate(callSignatures, getSignaturesOfType(instantiatedBaseType, SignatureKind.Call));
 constructSignatures = concatenate(constructSignatures, getSignaturesOfType(instantiatedBaseType, SignatureKind.Construct));
 stringIndexInfo = stringIndexInfo || getIndexInfoOfType(instantiatedBaseType, IndexKind.String);
 stringIndexInfo = stringIndexInfo || (instantiatedBaseType === anyType ?
@yuitYui (yuit)Apr 5, 2017

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

nit: would it be easier to read this way:
if(!stringIndexInfo){stringIndexInfo=instantiatedBaseType===anyType ? createIndexInfo(anyType,/*isReadonly*/false) : getIndexInfoOfType(instantiatedBaseType,IndexKind.String);}
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I guess so. Done.
@ghost
Deleted user (ghost)
commented
Apr 5, 2017•

edited by ghost

Loading

Copy link


Copy Markdown

I wonder if anything from #12352 could be simplified now.

At least the comment about getBaseConstructorTypeOfClass could be updated.
ghost
Deleted user (ghost)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 baseType = getTypeFromClassOrInterfaceReference(baseTypeNode, baseConstructorType.symbol);
 }
 else if (baseConstructorType.flags & TypeFlags.Any) {
 baseType = anyType;
@ghostDeleted user (ghost)Apr 5, 2017

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Could you leave a comment saying why it is necessary to set it to anyType instead of baseConstructorType?
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

There's no good reason; in fact it reduces test churn to return baseConstructorType for tests that extend undefined variables, which I think get unknownType instead of anyType, meaning that you get slightly more errors.
I'll change it to baseConstructorType.
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Andy (Andrewkraft) (@Andy-MS) Maybe? I don't understand the differences between using unknownSymbol (which I guess has unknownType) for a missing module vs creating a synthetic symbol. Did the synthetic symbol get created solely for error reporting?
Seems like it would be worth reverting #12352 to see how it behaves, because I'm not able to predict much about module behaviour a priori.
@ghost
Deleted user (ghost)
commented
Apr 5, 2017

Copy link


Copy Markdown

OK, we can try reverting #12352 later.

But please update this comment:
// This must be different than unknownSymbol because getBaseConstructorTypeOfClass won't fail for unknownSymbol.

Closed

@sandersn
Remove special symbol for untyped modules
90d5c29
Extending symbols from untyped modules is no longer an error, so #12532
didn't get us anything except slightly better quick info.
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Reverting #12352 turned out to be straightforward, so I ended up merging that branch back into this PR.
ghost
Deleted user (ghost)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
 const name = specifier.propertyName || specifier.name;
 if (name.text) {
 if (isUntypedOrShorthandAmbientModuleSymbol(moduleSymbol)) {
 if (isShorthandAmbientModuleSymbol(moduleSymbol)) {
@ghostDeleted user (ghost)Apr 5, 2017•

edited by ghost

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I like the old name better, even if it's a bit long.
@sandersn
Improve test and comments
f635042
Test asserts that unused locals error works for untyped modules.
Comment no longer claims to check for untyped modules.
@sandersn
Nathan Shively-Sanders (sandersn)
deleted the

allow-extending-from-any

branch
April 6, 2017 16:18
@ghostghost
mentioned this pull request
Jul 8, 2017

Closed

@microsoftMicrosoft (microsoft)
locked and limited conversation to collaborators
Jun 21, 2018
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser)DanielRosenwasser left review comments

+3 more reviewers

@alfaprojectJoão Dias (alfaproject)alfaproject left review comments
@yuitYui (yuit)yuit approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@sandersn@alfaproject@yuit@DanielRosenwasser@msftclas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Allow extending from any - #14935
Merged
Nathan Shively-Sanders (sandersn) merged 12 commits into
masterfrom 
allow-extending-from-any
Apr 6, 2017
Merged
Allow extending from any#14935
Nathan Shively-Sanders (sandersn) merged 12 commits into
masterfrom 
allow-extending-from-any
Conversation
@sandersn
@sandersnNathan Shively-Sanders (sandersn)
commented
Mar 30, 2017•

edited

Loading

Copy link


Copy Markdown

Member
Extending from any adds an index signature: [s: string]: any to both the

instance and static sides of the class.
Fixes#14301
Fixes#15040
@sandersn
Allow extending from any
f1c1969
Extending from any adds an index signature: [s: string]: any to both the
instance and static sides of the class.
@sandersn
Improve handling of any as a base type
765ac7c
Also improve how the string indexer for any-inheriting types is added.
@sandersn
Test: extending from any
650cc29
@sandersn
Update baselines
3624417
@sandersn
Re-swap mistakenly swapped lines of code
41eabbc
alfaproject
João Dias (alfaproject)
reviewed
Mar 31, 2017
View reviewed changes
Comment threadtests/baselines/reference/extendFromAny.js
Outdated

}

let c = new C();
c.known.length; // error, 'real' has no 'length' property
@alfaprojectJoão Dias (alfaproject)Mar 31, 2017

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

real / sreal
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Yui (@yuit)Andy (Andrewkraft) (@Andy-MS) mind taking a look?
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 return type.resolvedBaseConstructorType = unknownType;
 }
 if (baseConstructorType !== unknownType && baseConstructorType !== nullWideningType && !isConstructorType(baseConstructorType)) {
 if (baseConstructorType !== anyType && baseConstructorType !== unknownType && baseConstructorType !== nullWideningType && !isConstructorType(baseConstructorType)) {
@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser)Apr 5, 2017•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Hard for me to look up right now whether unknownType includes it, could you check for TypeFlags.Any?
@yuitYui (yuit)Apr 5, 2017

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just check unknownType and anyType both have TypeFlags.Any
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Includes anyType, autoType, unknownType. Seems like it should work.
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

done
yuit
Yui (yuit)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 function isValidBaseType(type: Type): boolean {
 return type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive) && !isGenericMappedType(type) ||
 return !!(type.flags & TypeFlags.Any) ||
 type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive) && !isGenericMappedType(type) ||
@yuitYui (yuit)Apr 5, 2017•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

should this just be type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive | TypeFlags.Any)
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I think it should work, although it's kind of a weird pun in that we rely on !isGenericMappedType(anyType) being true.
yuit
Yui (yuit)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 callSignatures = concatenate(callSignatures, getSignaturesOfType(instantiatedBaseType, SignatureKind.Call));
 constructSignatures = concatenate(constructSignatures, getSignaturesOfType(instantiatedBaseType, SignatureKind.Construct));
 stringIndexInfo = stringIndexInfo || getIndexInfoOfType(instantiatedBaseType, IndexKind.String);
 stringIndexInfo = stringIndexInfo || (instantiatedBaseType === anyType ?
@yuitYui (yuit)Apr 5, 2017

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

nit: would it be easier to read this way:
if(!stringIndexInfo){stringIndexInfo=instantiatedBaseType===anyType ? createIndexInfo(anyType,/*isReadonly*/false) : getIndexInfoOfType(instantiatedBaseType,IndexKind.String);}
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I guess so. Done.
@ghost
Deleted user (ghost)
commented
Apr 5, 2017•

edited by ghost

Loading

Copy link


Copy Markdown

I wonder if anything from #12352 could be simplified now.

At least the comment about getBaseConstructorTypeOfClass could be updated.
ghost
Deleted user (ghost)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 baseType = getTypeFromClassOrInterfaceReference(baseTypeNode, baseConstructorType.symbol);
 }
 else if (baseConstructorType.flags & TypeFlags.Any) {
 baseType = anyType;
@ghostDeleted user (ghost)Apr 5, 2017

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Could you leave a comment saying why it is necessary to set it to anyType instead of baseConstructorType?
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

There's no good reason; in fact it reduces test churn to return baseConstructorType for tests that extend undefined variables, which I think get unknownType instead of anyType, meaning that you get slightly more errors.
I'll change it to baseConstructorType.
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Andy (Andrewkraft) (@Andy-MS) Maybe? I don't understand the differences between using unknownSymbol (which I guess has unknownType) for a missing module vs creating a synthetic symbol. Did the synthetic symbol get created solely for error reporting?
Seems like it would be worth reverting #12352 to see how it behaves, because I'm not able to predict much about module behaviour a priori.
@ghost
Deleted user (ghost)
commented
Apr 5, 2017

Copy link


Copy Markdown

OK, we can try reverting #12352 later.

But please update this comment:
// This must be different than unknownSymbol because getBaseConstructorTypeOfClass won't fail for unknownSymbol.

Closed

@sandersn
Remove special symbol for untyped modules
90d5c29
Extending symbols from untyped modules is no longer an error, so #12532
didn't get us anything except slightly better quick info.
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Reverting #12352 turned out to be straightforward, so I ended up merging that branch back into this PR.
ghost
Deleted user (ghost)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
 const name = specifier.propertyName || specifier.name;
 if (name.text) {
 if (isUntypedOrShorthandAmbientModuleSymbol(moduleSymbol)) {
 if (isShorthandAmbientModuleSymbol(moduleSymbol)) {
@ghostDeleted user (ghost)Apr 5, 2017•

edited by ghost

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I like the old name better, even if it's a bit long.
@sandersn
Improve test and comments
f635042
Test asserts that unused locals error works for untyped modules.
Comment no longer claims to check for untyped modules.
@sandersn
Nathan Shively-Sanders (sandersn)
deleted the

allow-extending-from-any

branch
April 6, 2017 16:18
@ghostghost
mentioned this pull request
Jul 8, 2017

Closed

@microsoftMicrosoft (microsoft)
locked and limited conversation to collaborators
Jun 21, 2018
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser)DanielRosenwasser left review comments

+3 more reviewers

@alfaprojectJoão Dias (alfaproject)alfaproject left review comments
@yuitYui (yuit)yuit approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@sandersn@alfaproject@yuit@DanielRosenwasser@msftclas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Allow extending from any - #14935
Merged
Nathan Shively-Sanders (sandersn) merged 12 commits into
masterfrom 
allow-extending-from-any
Apr 6, 2017
Merged
Allow extending from any#14935
Nathan Shively-Sanders (sandersn) merged 12 commits into
masterfrom 
allow-extending-from-any
Conversation
@sandersn
@sandersnNathan Shively-Sanders (sandersn)
commented
Mar 30, 2017•

edited

Loading

Copy link


Copy Markdown

Member
Extending from any adds an index signature: [s: string]: any to both the

instance and static sides of the class.
Fixes#14301
Fixes#15040
@sandersn
Allow extending from any
f1c1969
Extending from any adds an index signature: [s: string]: any to both the
instance and static sides of the class.
@sandersn
Improve handling of any as a base type
765ac7c
Also improve how the string indexer for any-inheriting types is added.
@sandersn
Test: extending from any
650cc29
@sandersn
Update baselines
3624417
@sandersn
Re-swap mistakenly swapped lines of code
41eabbc
alfaproject
João Dias (alfaproject)
reviewed
Mar 31, 2017
View reviewed changes
Comment threadtests/baselines/reference/extendFromAny.js
Outdated

}

let c = new C();
c.known.length; // error, 'real' has no 'length' property
@alfaprojectJoão Dias (alfaproject)Mar 31, 2017

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

real / sreal
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Yui (@yuit)Andy (Andrewkraft) (@Andy-MS) mind taking a look?
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 return type.resolvedBaseConstructorType = unknownType;
 }
 if (baseConstructorType !== unknownType && baseConstructorType !== nullWideningType && !isConstructorType(baseConstructorType)) {
 if (baseConstructorType !== anyType && baseConstructorType !== unknownType && baseConstructorType !== nullWideningType && !isConstructorType(baseConstructorType)) {
@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser)Apr 5, 2017•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Hard for me to look up right now whether unknownType includes it, could you check for TypeFlags.Any?
@yuitYui (yuit)Apr 5, 2017

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just check unknownType and anyType both have TypeFlags.Any
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Includes anyType, autoType, unknownType. Seems like it should work.
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

done
yuit
Yui (yuit)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 function isValidBaseType(type: Type): boolean {
 return type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive) && !isGenericMappedType(type) ||
 return !!(type.flags & TypeFlags.Any) ||
 type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive) && !isGenericMappedType(type) ||
@yuitYui (yuit)Apr 5, 2017•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

should this just be type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive | TypeFlags.Any)
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I think it should work, although it's kind of a weird pun in that we rely on !isGenericMappedType(anyType) being true.
yuit
Yui (yuit)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 callSignatures = concatenate(callSignatures, getSignaturesOfType(instantiatedBaseType, SignatureKind.Call));
 constructSignatures = concatenate(constructSignatures, getSignaturesOfType(instantiatedBaseType, SignatureKind.Construct));
 stringIndexInfo = stringIndexInfo || getIndexInfoOfType(instantiatedBaseType, IndexKind.String);
 stringIndexInfo = stringIndexInfo || (instantiatedBaseType === anyType ?
@yuitYui (yuit)Apr 5, 2017

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

nit: would it be easier to read this way:
if(!stringIndexInfo){stringIndexInfo=instantiatedBaseType===anyType ? createIndexInfo(anyType,/*isReadonly*/false) : getIndexInfoOfType(instantiatedBaseType,IndexKind.String);}
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I guess so. Done.
@ghost
Deleted user (ghost)
commented
Apr 5, 2017•

edited by ghost

Loading

Copy link


Copy Markdown

I wonder if anything from #12352 could be simplified now.

At least the comment about getBaseConstructorTypeOfClass could be updated.
ghost
Deleted user (ghost)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 baseType = getTypeFromClassOrInterfaceReference(baseTypeNode, baseConstructorType.symbol);
 }
 else if (baseConstructorType.flags & TypeFlags.Any) {
 baseType = anyType;
@ghostDeleted user (ghost)Apr 5, 2017

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Could you leave a comment saying why it is necessary to set it to anyType instead of baseConstructorType?
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

There's no good reason; in fact it reduces test churn to return baseConstructorType for tests that extend undefined variables, which I think get unknownType instead of anyType, meaning that you get slightly more errors.
I'll change it to baseConstructorType.
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Andy (Andrewkraft) (@Andy-MS) Maybe? I don't understand the differences between using unknownSymbol (which I guess has unknownType) for a missing module vs creating a synthetic symbol. Did the synthetic symbol get created solely for error reporting?
Seems like it would be worth reverting #12352 to see how it behaves, because I'm not able to predict much about module behaviour a priori.
@ghost
Deleted user (ghost)
commented
Apr 5, 2017

Copy link


Copy Markdown

OK, we can try reverting #12352 later.

But please update this comment:
// This must be different than unknownSymbol because getBaseConstructorTypeOfClass won't fail for unknownSymbol.

Closed

@sandersn
Remove special symbol for untyped modules
90d5c29
Extending symbols from untyped modules is no longer an error, so #12532
didn't get us anything except slightly better quick info.
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Reverting #12352 turned out to be straightforward, so I ended up merging that branch back into this PR.
ghost
Deleted user (ghost)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
 const name = specifier.propertyName || specifier.name;
 if (name.text) {
 if (isUntypedOrShorthandAmbientModuleSymbol(moduleSymbol)) {
 if (isShorthandAmbientModuleSymbol(moduleSymbol)) {
@ghostDeleted user (ghost)Apr 5, 2017•

edited by ghost

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I like the old name better, even if it's a bit long.
@sandersn
Improve test and comments
f635042
Test asserts that unused locals error works for untyped modules.
Comment no longer claims to check for untyped modules.
@sandersn
Nathan Shively-Sanders (sandersn)
deleted the

allow-extending-from-any

branch
April 6, 2017 16:18
@ghostghost
mentioned this pull request
Jul 8, 2017

Closed

@microsoftMicrosoft (microsoft)
locked and limited conversation to collaborators
Jun 21, 2018
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser)DanielRosenwasser left review comments

+3 more reviewers

@alfaprojectJoão Dias (alfaproject)alfaproject left review comments
@yuitYui (yuit)yuit approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@sandersn@alfaproject@yuit@DanielRosenwasser@msftclas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Allow extending from any - #14935
Merged
Nathan Shively-Sanders (sandersn) merged 12 commits into
masterfrom 
allow-extending-from-any
Apr 6, 2017
Merged
Allow extending from any#14935
Nathan Shively-Sanders (sandersn) merged 12 commits into
masterfrom 
allow-extending-from-any
Conversation
@sandersn
@sandersnNathan Shively-Sanders (sandersn)
commented
Mar 30, 2017•

edited

Loading

Copy link


Copy Markdown

Member
Extending from any adds an index signature: [s: string]: any to both the

instance and static sides of the class.
Fixes#14301
Fixes#15040
@sandersn
Allow extending from any
f1c1969
Extending from any adds an index signature: [s: string]: any to both the
instance and static sides of the class.
@sandersn
Improve handling of any as a base type
765ac7c
Also improve how the string indexer for any-inheriting types is added.
@sandersn
Test: extending from any
650cc29
@sandersn
Update baselines
3624417
@sandersn
Re-swap mistakenly swapped lines of code
41eabbc
alfaproject
João Dias (alfaproject)
reviewed
Mar 31, 2017
View reviewed changes
Comment threadtests/baselines/reference/extendFromAny.js
Outdated

}

let c = new C();
c.known.length; // error, 'real' has no 'length' property
@alfaprojectJoão Dias (alfaproject)Mar 31, 2017

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

real / sreal
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Yui (@yuit)Andy (Andrewkraft) (@Andy-MS) mind taking a look?
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 return type.resolvedBaseConstructorType = unknownType;
 }
 if (baseConstructorType !== unknownType && baseConstructorType !== nullWideningType && !isConstructorType(baseConstructorType)) {
 if (baseConstructorType !== anyType && baseConstructorType !== unknownType && baseConstructorType !== nullWideningType && !isConstructorType(baseConstructorType)) {
@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser)Apr 5, 2017•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Hard for me to look up right now whether unknownType includes it, could you check for TypeFlags.Any?
@yuitYui (yuit)Apr 5, 2017

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just check unknownType and anyType both have TypeFlags.Any
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Includes anyType, autoType, unknownType. Seems like it should work.
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

done
yuit
Yui (yuit)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 function isValidBaseType(type: Type): boolean {
 return type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive) && !isGenericMappedType(type) ||
 return !!(type.flags & TypeFlags.Any) ||
 type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive) && !isGenericMappedType(type) ||
@yuitYui (yuit)Apr 5, 2017•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

should this just be type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive | TypeFlags.Any)
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I think it should work, although it's kind of a weird pun in that we rely on !isGenericMappedType(anyType) being true.
yuit
Yui (yuit)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 callSignatures = concatenate(callSignatures, getSignaturesOfType(instantiatedBaseType, SignatureKind.Call));
 constructSignatures = concatenate(constructSignatures, getSignaturesOfType(instantiatedBaseType, SignatureKind.Construct));
 stringIndexInfo = stringIndexInfo || getIndexInfoOfType(instantiatedBaseType, IndexKind.String);
 stringIndexInfo = stringIndexInfo || (instantiatedBaseType === anyType ?
@yuitYui (yuit)Apr 5, 2017

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

nit: would it be easier to read this way:
if(!stringIndexInfo){stringIndexInfo=instantiatedBaseType===anyType ? createIndexInfo(anyType,/*isReadonly*/false) : getIndexInfoOfType(instantiatedBaseType,IndexKind.String);}
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I guess so. Done.
@ghost
Deleted user (ghost)
commented
Apr 5, 2017•

edited by ghost

Loading

Copy link


Copy Markdown

I wonder if anything from #12352 could be simplified now.

At least the comment about getBaseConstructorTypeOfClass could be updated.
ghost
Deleted user (ghost)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 baseType = getTypeFromClassOrInterfaceReference(baseTypeNode, baseConstructorType.symbol);
 }
 else if (baseConstructorType.flags & TypeFlags.Any) {
 baseType = anyType;
@ghostDeleted user (ghost)Apr 5, 2017

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Could you leave a comment saying why it is necessary to set it to anyType instead of baseConstructorType?
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

There's no good reason; in fact it reduces test churn to return baseConstructorType for tests that extend undefined variables, which I think get unknownType instead of anyType, meaning that you get slightly more errors.
I'll change it to baseConstructorType.
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Andy (Andrewkraft) (@Andy-MS) Maybe? I don't understand the differences between using unknownSymbol (which I guess has unknownType) for a missing module vs creating a synthetic symbol. Did the synthetic symbol get created solely for error reporting?
Seems like it would be worth reverting #12352 to see how it behaves, because I'm not able to predict much about module behaviour a priori.
@ghost
Deleted user (ghost)
commented
Apr 5, 2017

Copy link


Copy Markdown

OK, we can try reverting #12352 later.

But please update this comment:
// This must be different than unknownSymbol because getBaseConstructorTypeOfClass won't fail for unknownSymbol.

Closed

@sandersn
Remove special symbol for untyped modules
90d5c29
Extending symbols from untyped modules is no longer an error, so #12532
didn't get us anything except slightly better quick info.
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Reverting #12352 turned out to be straightforward, so I ended up merging that branch back into this PR.
ghost
Deleted user (ghost)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
 const name = specifier.propertyName || specifier.name;
 if (name.text) {
 if (isUntypedOrShorthandAmbientModuleSymbol(moduleSymbol)) {
 if (isShorthandAmbientModuleSymbol(moduleSymbol)) {
@ghostDeleted user (ghost)Apr 5, 2017•

edited by ghost

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I like the old name better, even if it's a bit long.
@sandersn
Improve test and comments
f635042
Test asserts that unused locals error works for untyped modules.
Comment no longer claims to check for untyped modules.
@sandersn
Nathan Shively-Sanders (sandersn)
deleted the

allow-extending-from-any

branch
April 6, 2017 16:18
@ghostghost
mentioned this pull request
Jul 8, 2017

Closed

@microsoftMicrosoft (microsoft)
locked and limited conversation to collaborators
Jun 21, 2018
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser)DanielRosenwasser left review comments

+3 more reviewers

@alfaprojectJoão Dias (alfaproject)alfaproject left review comments
@yuitYui (yuit)yuit approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@sandersn@alfaproject@yuit@DanielRosenwasser@msftclas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Allow extending from any - #14935
Merged
Nathan Shively-Sanders (sandersn) merged 12 commits into
masterfrom 
allow-extending-from-any
Apr 6, 2017
Merged
Allow extending from any#14935
Nathan Shively-Sanders (sandersn) merged 12 commits into
masterfrom 
allow-extending-from-any
Conversation
@sandersn
@sandersnNathan Shively-Sanders (sandersn)
commented
Mar 30, 2017•

edited

Loading

Copy link


Copy Markdown

Member
Extending from any adds an index signature: [s: string]: any to both the

instance and static sides of the class.
Fixes#14301
Fixes#15040
@sandersn
Allow extending from any
f1c1969
Extending from any adds an index signature: [s: string]: any to both the
instance and static sides of the class.
@sandersn
Improve handling of any as a base type
765ac7c
Also improve how the string indexer for any-inheriting types is added.
@sandersn
Test: extending from any
650cc29
@sandersn
Update baselines
3624417
@sandersn
Re-swap mistakenly swapped lines of code
41eabbc
alfaproject
João Dias (alfaproject)
reviewed
Mar 31, 2017
View reviewed changes
Comment threadtests/baselines/reference/extendFromAny.js
Outdated

}

let c = new C();
c.known.length; // error, 'real' has no 'length' property
@alfaprojectJoão Dias (alfaproject)Mar 31, 2017

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

real / sreal
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Yui (@yuit)Andy (Andrewkraft) (@Andy-MS) mind taking a look?
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 return type.resolvedBaseConstructorType = unknownType;
 }
 if (baseConstructorType !== unknownType && baseConstructorType !== nullWideningType && !isConstructorType(baseConstructorType)) {
 if (baseConstructorType !== anyType && baseConstructorType !== unknownType && baseConstructorType !== nullWideningType && !isConstructorType(baseConstructorType)) {
@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser)Apr 5, 2017•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Hard for me to look up right now whether unknownType includes it, could you check for TypeFlags.Any?
@yuitYui (yuit)Apr 5, 2017

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just check unknownType and anyType both have TypeFlags.Any
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Includes anyType, autoType, unknownType. Seems like it should work.
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

done
yuit
Yui (yuit)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 function isValidBaseType(type: Type): boolean {
 return type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive) && !isGenericMappedType(type) ||
 return !!(type.flags & TypeFlags.Any) ||
 type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive) && !isGenericMappedType(type) ||
@yuitYui (yuit)Apr 5, 2017•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

should this just be type.flags & (TypeFlags.Object | TypeFlags.NonPrimitive | TypeFlags.Any)
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I think it should work, although it's kind of a weird pun in that we rely on !isGenericMappedType(anyType) being true.
yuit
Yui (yuit)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 callSignatures = concatenate(callSignatures, getSignaturesOfType(instantiatedBaseType, SignatureKind.Call));
 constructSignatures = concatenate(constructSignatures, getSignaturesOfType(instantiatedBaseType, SignatureKind.Construct));
 stringIndexInfo = stringIndexInfo || getIndexInfoOfType(instantiatedBaseType, IndexKind.String);
 stringIndexInfo = stringIndexInfo || (instantiatedBaseType === anyType ?
@yuitYui (yuit)Apr 5, 2017

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

nit: would it be easier to read this way:
if(!stringIndexInfo){stringIndexInfo=instantiatedBaseType===anyType ? createIndexInfo(anyType,/*isReadonly*/false) : getIndexInfoOfType(instantiatedBaseType,IndexKind.String);}
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I guess so. Done.
@ghost
Deleted user (ghost)
commented
Apr 5, 2017•

edited by ghost

Loading

Copy link


Copy Markdown

I wonder if anything from #12352 could be simplified now.

At least the comment about getBaseConstructorTypeOfClass could be updated.
ghost
Deleted user (ghost)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
Outdated

 baseType = getTypeFromClassOrInterfaceReference(baseTypeNode, baseConstructorType.symbol);
 }
 else if (baseConstructorType.flags & TypeFlags.Any) {
 baseType = anyType;
@ghostDeleted user (ghost)Apr 5, 2017

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Could you leave a comment saying why it is necessary to set it to anyType instead of baseConstructorType?
@sandersnNathan Shively-Sanders (sandersn)Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

There's no good reason; in fact it reduces test churn to return baseConstructorType for tests that extend undefined variables, which I think get unknownType instead of anyType, meaning that you get slightly more errors.
I'll change it to baseConstructorType.
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Andy (Andrewkraft) (@Andy-MS) Maybe? I don't understand the differences between using unknownSymbol (which I guess has unknownType) for a missing module vs creating a synthetic symbol. Did the synthetic symbol get created solely for error reporting?
Seems like it would be worth reverting #12352 to see how it behaves, because I'm not able to predict much about module behaviour a priori.
@ghost
Deleted user (ghost)
commented
Apr 5, 2017

Copy link


Copy Markdown

OK, we can try reverting #12352 later.

But please update this comment:
// This must be different than unknownSymbol because getBaseConstructorTypeOfClass won't fail for unknownSymbol.

Closed

@sandersn
Remove special symbol for untyped modules
90d5c29
Extending symbols from untyped modules is no longer an error, so #12532
didn't get us anything except slightly better quick info.
@sandersn
Nathan Shively-Sanders (sandersn)
commented
Apr 5, 2017

Copy link


Copy Markdown

MemberAuthor
Reverting #12352 turned out to be straightforward, so I ended up merging that branch back into this PR.
ghost
Deleted user (ghost)
reviewed
Apr 5, 2017
View reviewed changes
Comment threadsrc/compiler/checker.ts
 const name = specifier.propertyName || specifier.name;
 if (name.text) {
 if (isUntypedOrShorthandAmbientModuleSymbol(moduleSymbol)) {
 if (isShorthandAmbientModuleSymbol(moduleSymbol)) {
@ghostDeleted user (ghost)Apr 5, 2017•

edited by ghost

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I like the old name better, even if it's a bit long.
@sandersn
Improve test and comments
f635042
Test asserts that unused locals error works for untyped modules.
Comment no longer claims to check for untyped modules.
@sandersn
Nathan Shively-Sanders (sandersn)
deleted the

allow-extending-from-any

branch
April 6, 2017 16:18
@ghostghost
mentioned this pull request
Jul 8, 2017

Closed

@microsoftMicrosoft (microsoft)
locked and limited conversation to collaborators
Jun 21, 2018
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser)DanielRosenwasser left review comments

+3 more reviewers

@alfaprojectJoão Dias (alfaproject)alfaproject left review comments
@yuitYui (yuit)yuit approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@sandersn@alfaproject@yuit@DanielRosenwasser@msftclas