Skip to content

vscode extension: migrate from any to unknown where possible #2989

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

Merged
merged 3 commits into from
Feb 2, 2020

Conversation

Veetaha
Copy link
Contributor

@Veetaha Veetaha commented Feb 2, 2020

unknown type is the stricter version of any and it should always be prefered (like const over let).
It lets you assign any value to it, but doesn't let you carry out arbitrary operations on them without an explicit type check (like typeof unknownValue === 'string').

@Veetaha Veetaha changed the title vscode extension: migrate from any to unknown where possible [WIP] vscode extension: migrate from any to unknown where possible Feb 2, 2020
@Veetaha Veetaha requested a review from matklad February 2, 2020 20:37
@Veetaha Veetaha changed the title [WIP] vscode extension: migrate from any to unknown where possible vscode extension: migrate from any to unknown where possible Feb 2, 2020
Copy link
Member

@matklad matklad left a comment

Choose a reason for hiding this comment

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

lgmt

@@ -55,7 +55,7 @@ export function syntaxTree(ctx: Ctx): Cmd {

// We need to order this after LS updates, but there's no API for that.
// Hence, good old setTimeout.
function afterLs(f: () => any) {
function afterLs(f: () => unknown) {
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this maybe should be => void, or whats the TypeScript alternative for -> ()?

Copy link
Member

Choose a reason for hiding this comment

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

Also, this is just a horrible hack which needs to die in flames...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, let me revisit this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right, void is semantically -> (), but not the unit type itself (). It is quite a special language item that is meant to be used only for the function return type and denotes that the value should be discarded (also function with void return type is covariant to other functions with different return type, in which way void type is different from undefined type).

I would say that the concept of unit type the way it is present in Rust is not implemented in TypeScripts type system.

return this.extCtx.subscriptions;
}

pushCleanup(d: { dispose(): any }) {
pushCleanup(d: { dispose(): unknown }) {
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, this wants to be a : void?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, yes you are right, now when I am looking at it, this seems reasonable

@matklad
Copy link
Member

matklad commented Feb 2, 2020

bors d+

@bors
Copy link
Contributor

bors bot commented Feb 2, 2020

✌️ Veetaha can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@Veetaha
Copy link
Contributor Author

Veetaha commented Feb 2, 2020

@matklad, I've noticed, that we don't invoke type checking at CI. I am not familiar with rollup at all (it is not popular bundler, the most popular is webpack), but it seems that it does not leverage the TypeScript type checker while bundling.

@Veetaha
Copy link
Contributor Author

Veetaha commented Feb 2, 2020

The easiest fix for that would be to do npx tsc --noEmit

@matklad
Copy link
Member

matklad commented Feb 2, 2020

Hm, yeah, we should make sure that rollup typechecks stuff

@Veetaha Veetaha force-pushed the feature/refactoring-vscode-ext branch from 6926d8e to 2fd7af2 Compare February 2, 2020 21:24
@Veetaha
Copy link
Contributor Author

Veetaha commented Feb 2, 2020

bors r+

bors bot added a commit that referenced this pull request Feb 2, 2020
2989: vscode extension: migrate from any to unknown where possible r=Veetaha a=Veetaha

`unknown` type is the stricter version of `any` and it should always be prefered (like `const` over `let`).
It lets you assign any value to it, but doesn't let you carry out arbitrary operations on them without an explicit type check (like `typeof unknownValue === 'string'`).

Co-authored-by: Veetaha <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 2, 2020

Build succeeded

  • Rust (macos-latest)
  • Rust (ubuntu-latest)
  • Rust (windows-latest)
  • TypeScript

@bors bors bot merged commit 2fd7af2 into rust-lang:master Feb 2, 2020
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
miri: implement some `llvm.x86.sse.*` intrinsics and add tests

PR moved from rust-lang/rust#113932.

Implements LLVM intrisics needed to run most SSE functions from `core::arch::x86{,_64}`.

Also adds miri tests for those functions (mostly copied from core_arch tests).

r? `@RalfJung`

The first commit is the same that the commit in the PR I had opened in the Rust repository. I addressed review comments in additional commits to make it easier to review. I also fixed formatting and clippy warnings.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
miri: implement some `llvm.x86.sse.*` intrinsics and add tests

PR moved from rust-lang/rust#113932.

Implements LLVM intrisics needed to run most SSE functions from `core::arch::x86{,_64}`.

Also adds miri tests for those functions (mostly copied from core_arch tests).

r? `@RalfJung`

The first commit is the same that the commit in the PR I had opened in the Rust repository. I addressed review comments in additional commits to make it easier to review. I also fixed formatting and clippy warnings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants