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

RFC: Fix an unsoundness issue around stripping optional properties #276

Conversation

asajeffrey
Copy link
Collaborator

Two RFCs that combine to fix an unsoundness issue with table subtyping.

@asajeffrey asajeffrey added the rfc Language change proposal label Dec 3, 2021
@asajeffrey asajeffrey changed the title Fix an unsoundness issue around stripping optional properties RFC: Fix an unsoundness issue around stripping optional properties Dec 3, 2021
Copy link

@Nolan-O Nolan-O left a comment

Choose a reason for hiding this comment

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

I find it strange that { p: number } is considered a sealed type at any point in time. Adding any new indices to a table of that type will not pass typechecking unless done at declaration, and it seems that accessing the field in any way later will cause the declaration to not typecheck:

image

Could we not just evaluate this to an unsealed type? It was my understanding before testing this that the only way to declare sealed types is to provide a type of an indexer, and any specific keys in a type will make it sealed

@asajeffrey asajeffrey mentioned this pull request Dec 6, 2021
@asajeffrey
Copy link
Collaborator Author

.Could we not just evaluate this to an unsealed type? It was my understanding before testing this that the only way to declare sealed types is to provide a type of an indexer, and any specific keys in a type will make it sealed

User-supplied type annotations are always sealed. Type inference seals tables when they are returned from functions. (We should also be doing alias analysis to ensure unique access to unsealed tables, but that's another story.)

@zeux
Copy link
Collaborator

zeux commented Jan 4, 2022

Just a process note... we should probably use one RFC per PR. When two orthogonal features are submitted as one PR, it makes it impossible for us to merge one but not the other. When the features are coupled and must come together, they should ideally come as a single .md file. I'd recommend merging the .md files in this case, especially since they reference each other. (I understand that the RFC proposes two separate changes to the type system but since they both need to happen I think it's better to have fewer .md files and have 1-1 relationship).

@asajeffrey
Copy link
Collaborator Author

These really are different (but related) features, I'd like to keep them as two md files. I'll split into two PRs if you like.

@zeux
Copy link
Collaborator

zeux commented Jan 4, 2022

Can we at least remove cross-referencing then? Making all table literals unsealed seems to stand on its own at least.

@zeux
Copy link
Collaborator

zeux commented Jan 4, 2022

One downside to avoiding sealing non-empty table literals is that it leads to typos in assignment being difficult/impossible to detect:

local t = {x = 1, y = 2}
if foo then
t.z = 3 -- is z a typo or intentional 2-vs-3 choice?
end

Which I suppose is why we're sealing them currently. Of course it's unclear what to do about this in absence of type annotations or record types, so it's probably fine - the code above could easily be valid or invalid depending on the intent. My understanding is that we will still detect uses of keys that were never written to.

@asajeffrey
Copy link
Collaborator Author

Yes, not much we can do about that though. I'll add this point to the downsides section.

@asajeffrey
Copy link
Collaborator Author

I removed the cyclic cross-referencing, so the subtyping proposal refers to the table literals proposal, but not the other way round.

@alexmccord
Copy link
Contributor

Control flow analysis will be able to keep an eye on properties that are definitely-or-maybe assigned. The same problem still arise even when the tables are empty anyway regardless of this RFC.

local t = {}
if cond()
  t.x = 5
end

local some_number: number = t.x

Co-authored-by: vegorov-rbx <75688451+vegorov-rbx@users.noreply.github.com>
@asajeffrey asajeffrey merged commit 82587be into luau-lang:master Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc Language change proposal
Development

Successfully merging this pull request may close these issues.

8 participants