Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeScript incorrectly considers variable as possibly null inside condition that checks variable #619

Closed
cyberalien opened this issue Oct 18, 2020 · 21 comments
Labels
bug Something isn't working Fixed Fixed in master branch. Pending production release.

Comments

@cyberalien
Copy link

Describe the bug
TypeScript error "Object is possibly null" is shown for nested conditional checks.

To Reproduce
Simple test case:

<script lang="typescript">
	interface TestObject {
		author?: string;
	}
	export let test: TestObject | null;
</script>

{#if test}
    <div>
        {#if test.author}
	        <div>Author: {test.author}</div>
	    {/if}
    </div>
{/if}

TypeScript throws error Object is possibly null on line {#if test.author}, not specifically on test inside that line.

Expected behavior
Expected not to throw any errors.

System (please complete the following information):

  • OS: OSX 10.15.7
  • IDE: VSCode
  • Plugin/Package: "Svelte for VSCode"

Additional context
If nested conditional statement is removed and in TestObject interface author? is replaced with author (to make it required), it would be logical for TypeScript to throw the same error for {test.author}, but it doesn't. So looks like error is triggered by nested conditional statement, without it TypeScript knows that test is not null.

@cyberalien cyberalien added the bug Something isn't working label Oct 18, 2020
@jasonlyu123
Copy link
Member

Don't know what can we do to deal with it. svelte2tsx transform it into this:

() => (<>

{() => {if (test){<>
    <div>
        {() => {if (test.author){<>
	        <div>Author: {test.author}</div>
	    </>}}}
    </div>
</>}}}</>);

because it's wrapped in a function so that the control flow type analysis won't take into effect. And we can't remove the function or wrapped into a immediately-invoked function because the expression would re-run when it's dependency cahnged, we definitely needs to invalidate some of the control flow type.

@dummdidumm
Copy link
Member

Could we turn this into ternaries? If there's not else block we just use an empty string for the else case.
But the bigger problem might be that we are not allowed to use functions like that in any of the transformations - not sure if that's the case right now

@cyberalien
Copy link
Author

Maybe allow ! syntax? {#if test!.author}. That's valid TypeScript syntax, but right now it cannot be used in template.

@jasonlyu123
Copy link
Member

jasonlyu123 commented Oct 19, 2020

Right now you can't use typescript in markup. There's no way to preprocess markup or let compiler parse typescript syntax. So non-null assertion is not possible.
Haven't tried the viability buy maybe we could copy the upper-level condition and append it before the nested condition

 {() => {if (test && test.author){<>
	        <div>Author: {test.author}</div>
	    </>}}}

@cyberalien
Copy link
Author

cyberalien commented Oct 22, 2020

Temporary workaround:

<script lang="typescript">
	interface TestObject {
		author?: string;
	}
	export let test: TestObject | null;

	let requitedTest: Required<TestObject>;
	$: {
		requiredTest = test as unknown as Required<TestObject>;
	}
</script>

{#if test}
    <div>
        {#if requiredTest.author}
	        <div>Author: {requiredTest.author}</div>
	    {/if}
    </div>
{/if}

Copying variable, assigning different type and using it in nested condition. It is ugly and needs to be used carefully, but it does get rid of warnings.

@AlexGalays
Copy link

AlexGalays commented Oct 30, 2020

A more pragmatic workaround for now would be

{#if test}
    <div>
        {#if test?.author}
	        <div>Author: {test.author}</div>
	    {/if}
    </div>
{/if}

It's quite annoying, especially when it's not just drilling inside the same object but making separate conditions:

{#if bigSwitch}
    <div>
        {#if smallSwitch}
	        <MyComponent {bigSwitch} />
	    {/if}
    </div>
{/if}

@AlexGalays
Copy link

That won't work. TypeScript is not allowed in templates.

Seems to work here. test && test.author then.

@jasonlyu123
Copy link
Member

jasonlyu123 commented Oct 30, 2020

if you mean optional chaining, that's a new javascript feature, not just a typescript feature. It's available in svelte after 3.24.0

@cyberalien
Copy link
Author

Thanks! I always assumed it was TS feature similar to non-null statement foo!. That makes it much simpler.

Always learning something new :)

@dummdidumm
Copy link
Member

Haven't tried the viability buy maybe we could copy the upper-level condition and append it before the nested condition

 {() => {if (test && test.author){<>
	        <div>Author: {test.author}</div>
	    </>}}}

I did some tests in that direction and I think this will not work for all cases. It may silence errors for #if, but they will reappear as soon as you use an #each or #await block inside it. These errors will not appear if the variable is a const, because TS control flow then knows that the condition must still be true because the variable cannot be reassigned. So the best way to solve this would be to transform everything to const variables, or reassign all variables to temporary other variables for the template. That will bring its own problems though for hover info, rename, go-to-declaration etc. Maybe that could be worked around by using comma-expressions and transform {#if foo} to {#if (foo, generatedConstFoo)}. Bottom line: this will require quite some hackery.

@dummdidumm
Copy link
Member

Maybe the "append upper-level if-blocks"-thing is correct after all. It is not safe to assume that the conditions are still true inside each/await blocks. For example this will throw a runtime error (toUpperCase is not a function):

<script>
	let name = 'name';
	function setNameToNumber(){
		name = 1
		return ['a']
	}
</script>

{#if name}
	<h1>Hello {name.toUpperCase()}!</h1>
	{#each setNameToNumber() as item}
		item
	{/each}
{/if}

The same can be constructed for an await-block.

dummdidumm pushed a commit to dummdidumm/language-tools that referenced this issue Dec 5, 2020
sveltejs#619
This way the TypeScript control flow will be broken in less instances and nested if-blocks like
```
{#if a}
  {#if a.a}
     ..
  {/if}
{/if}
```
will not throw "a could be undefined" in the inner if anymore in strict mode.
This will not shut down all instances of control flow breakage, because for example slots/each/await are still transformed to lambda-functions.
dummdidumm added a commit that referenced this issue Dec 8, 2020
#619
This way the TypeScript control flow will be broken in less instances and nested if-blocks like
```
{#if a}
  {#if a.a}
     ..
  {/if}
{/if}
```
will not throw "a could be undefined" in the inner if anymore in strict mode.
This will not shut down all instances of control flow breakage, because for example slots/each/await are still transformed to lambda-functions.
@dummdidumm
Copy link
Member

Small update/summary:

  • control flow is reset as soon as the code is inside one of these constructs: #await, #each, let:X. This is due to the way we have to transform the code. We haven't forgotten about this issue and are still looking for ways to solves this in a good way.
  • nested if-conditions now should work as expected as long as they are not interrupted by one of the things mentioned in the first bullet point

@dummdidumm
Copy link
Member

Right now you can't use typescript in markup. There's no way to preprocess markup or let compiler parse typescript syntax. So non-null assertion is not possible.
Haven't tried the viability buy maybe we could copy the upper-level condition and append it before the nested condition

 {() => {if (test && test.author){<>
	        <div>Author: {test.author}</div>
	    </>}}}

Now that #493 is fixed in a way that declares $store-variables, we can revisit this idea. My idea is to prepend all if-conditions like this (#each as an example):

In

{#if somecondition && anothercondition}
   {#each ..}
     ..
   {/each}
{/if}

Out

<>{__sveltets_each((true, items), (item) => (somecondition && anothercondition) && <>
    ...
</>)}</>

The tricky part is to get elseif/else and nesting logic right.

dummdidumm pushed a commit to dummdidumm/language-tools that referenced this issue Feb 26, 2021
This adds an IfScope to svelte2tsx which keeps track of the if scope including nested ifs. That scope is used to prepend the resulting condition check to all inner lambda functions that svelte2tsx needs to create for slot, each, await. This way, TypeScript's control flow can determine the type of checked variables and no longer loses that info due to the lambda functions.
sveltejs#619
dummdidumm added a commit that referenced this issue Mar 10, 2021
This adds two new scopes to svelte2tsx: IfScope, which keeps track of the if scope including nested ifs, and TemplateScope, which tracks all initialized variables at certain AST levels (await, each, let:slot).. These scopes are used to prepend a repeated if-condition check to all inner lambda functions that svelte2tsx needs to create for slot, each, await. This way, TypeScript's control flow can determine the type of checked variables inside these lambda functions and no longer loses that info.
#619
@dummdidumm dummdidumm added the Fixed Fixed in master branch. Pending production release. label Mar 10, 2021
@dummdidumm
Copy link
Member

This should be fixed with VS Code version 104.6.3 / svelte-check version 1.2.4 . Please let me know if it works for you now.

@Liamolucko
Copy link
Contributor

I've found an issue - This works correctly inside templates now, but not in event listeners. I'm not sure if I should open a new issue for it, but here's a repro:

<script lang="ts">
  let value: string | null = null;

  function acceptsString(value: string) {
    console.log(value);
  }
</script>

{#if value}
  <!-- Argument of type 'string | null' is not assignable to parameter of type 'string'.
  Type 'null' is not assignable to type 'string'. -->
  <button on:click={() => acceptsString(value)} />
{/if}

@AlexGalays
Copy link

I've found an issue - This works correctly inside templates now, but not in event listeners. I'm not sure if I should open a new issue for it, but here's a repro:

<script lang="ts">
  let value: string | null = null;

  function acceptsString(value: string) {
    console.log(value);
  }
</script>

{#if value}
  <!-- Argument of type 'string | null' is not assignable to parameter of type 'string'.
  Type 'null' is not assignable to type 'string'. -->
  <button on:click={() => acceptsString(value)} />
{/if}

That's just the regular TS behavior though. Your value is referenced in a function that could be called any time in the future where value might be back to null.

@dummdidumm
Copy link
Member

Both are valid views. One could say "since value is not a const, it could change", but on the other hand one could also say "I never get to that click handler if value is null". Tricky ..

@dummdidumm
Copy link
Member

dummdidumm commented Mar 12, 2021

I'll open a different issue for that so we can discuss this seperately.

@AlexGalays
Copy link

I don't think it's worth your time to discuss TS's own, old choices :p
Wouldn't the button stick around with value===null if there is an out transition at their or their parent's level for instance?

@dummdidumm
Copy link
Member

dummdidumm commented Mar 12, 2021

Strictly speaking the control flow was correct before these relaxing changes because one could modify other variables within an each loop or an await.

<script lang="ts">
  let foo: string | null = 'bar';
  function getItems() {
     foo = null;
     return [''];
  }
</script>

{#if foo}
  {#each getItems() as item}
    {foo.toUpperCase()} <!-- boom -->
  {/each}
{/if}

.. but who does this? You can fabricate the same runtime errors in TS, too, btw. So it's more of a tradeoff in DX than "this is 100% correct".

But before we discuss this further in here, please take this discussion to #876 😄

@KuSh
Copy link

KuSh commented May 29, 2022

Hi,

These doesn't seems to work completely.
With the following code I have an Object is possibly 'undefined' ts(2532) error

Dependencies versions:

"svelte": "^3.48.0",
"svelte-check": "^2.7.1",
"svelte-preprocess": "^4.10.6",
"tslib": "^2.4.0",
"typescript": "^4.7.2"

and

Svelte for VS Code v105.16.1

<script lang="ts">
  export let content: {
    list: {
      image?: {
        src: string;
        alt: string;
      };
    }[];
  };
  export let item = 0;
</script>

<div>
  {#if content.list[item]?.image}
    <img
      src={content.list[item].image.src}
      alt={content.list[item].image.alt}
    />
  {/if}
</div>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Fixed Fixed in master branch. Pending production release.
Projects
None yet
Development

No branches or pull requests

6 participants