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

[Feature Request] Add a strong typed struct/class that cannot assign a new field that is not defined in the struct/class. #1990

Open
eminor1988 opened this issue Mar 8, 2023 · 63 comments
Labels
enhancement New feature or request

Comments

@eminor1988
Copy link

eminor1988 commented Mar 8, 2023

  • Example:
---@struct Student
---@field study_level integer

---@type Student
local new_student = {
    study_level = 0,
    tobacco_level = 1, -- This should be error.
}
  • Usage:
    We can do refactoring easier when rename the fields with this feature.
@sumneko sumneko added the enhancement New feature or request label Mar 8, 2023
@Mayron
Copy link

Mayron commented Apr 7, 2023

I'd like to see this too. I'm in favour of struct because it's similar to C#, but exact works fine too. I'm also liking other suggestions for built-in types like Exact<MyClass> instead of more @ styled notation because that might enable support for future features with a more modular approach.

For example, ReadOnly<T> could then also be added to change the behaviour of the wrapped generic type T where T is a collection data type (e.g., a table or list) to tell LuaLS that its read-only, or Callable<MyTable> might be good for declaring that a table has the __call meta-method so you can call it like a function. Who knows 🤷 but it sounds interesting.

@Calandiel
Copy link

Would you accept PRs regarding this or is this subject completely off the table?

@sumneko
Copy link
Collaborator

sumneko commented Apr 24, 2023

Would you accept PRs regarding this or is this subject completely off the table?

I'm glad to accept PRs.

@tmillr
Copy link
Contributor

tmillr commented May 18, 2023

Really need this feature (for one, it catches typos). It shouldn't be too hard to implement?

Referencing an undefined variable emits an LSP error or warning (even regardless of the fact that it is not a runtime error to do so) and this is a good thing, so why wouldn't the same hold true for table indices?

IIRC, typescript disallows extra keys by default (or maybe it's opt-in behavior to have it that way, I don't remember), and then you can allow additional arbitrary keys on a type by it defining/declaring like this:

type MyObj = {
    [key: string]: any;
    definedKey1: boolean;
    definedKey2: string;
    ...

instead of like this:

type MyObj = {
    definedKey1: boolean;
    definedKey2: string;
    ...

I don't think this is totally necessary however. A special symbol, type constructor, or additional annotation (kinda like @field is its own annotation and goes on its own line) would suffice (whether it's Freeze<> or Exact<> or ---@exact or whatever else).

@GiuseppeIII
Copy link

GiuseppeIII commented May 29, 2023

An exact table for the input of functions is also something I would also be very interested in! Initially I was just using params everywhere for function input definitions, but wanted to switch over to using exact dicts as the input to functions utilizing @Class, as I thought it made the code cleaner (especially in places where the arguments in should be 'nested').

One thing I would like to see though if this every makes it in, is the possibility to mark fields on classes as optional using the normal ? syntax. When using the @Class I want to require some fields, allow but not require some other fields, and disallow everything else

@sumneko
Copy link
Collaborator

sumneko commented May 31, 2023

My current idea is

---@class Student
---@exact
---@field study_level integer

or

---@class Student <exact>
---@field study_level integer

Any other good ideas are also welcome.

(Translated by ChatGPT)

@firas-assaad
Copy link
Contributor

I know it'll break existing code and require work to update the definitions everywhere, but would you consider simply changing the existing @class so that it enforces exactness by default? If a user wants to opt out for certain fields, they'll have to manually annotate them as having nil values:

---@field study_level? integer
-- or this, but I prefer the first syntax:
---@field study_level integer?

This is how TypeScript works and while Lua annotations and TypeScript are different, I think enforcing types to be exact by default is a more consistent design that forces the user to think about class fields and whether they should be required or optional.

Of course, I understand the amount of headache involved in updating large codebases and addon libraries, but I thought it's worth mentioning this option if you want to consider it for a future major version.

@lewis6991
Copy link
Contributor

I agree with @firas-assaad . As a compromise this could be enabled by default when only strict is enabled

@sumneko
Copy link
Collaborator

sumneko commented May 31, 2023

I know it'll break existing code and require work to update the definitions everywhere, but would you consider simply changing the existing so that it enforces exactness by default? If a user wants to opt out for certain fields, they'll have to manually annotate them as having nil values:@class

---@field study_level? integer
-- or this, but I prefer the first syntax:
---@field study_level integer?

This is how TypeScript works and while Lua annotations and TypeScript are different, I think enforcing types to be exact by default is a more consistent design that forces the user to think about class fields and whether they should be required or optional.

Of course, I understand the amount of headache involved in updating large codebases and addon libraries, but I thought it's worth mentioning this option if you want to consider it for a future major version.

No, because many users will declare fields using a mixed approach, for example:

---@class MyClass
---@field id integer
local M = Class 'MyClass'

M.showName = 'MyClass'

function M:init()
    self.list = {}
    self.id  = nextID()
end

(Translated by ChatGPT)

@firas-assaad
Copy link
Contributor

firas-assaad commented May 31, 2023

No, because many users will declare fields using a mixed approach, for example:

---@class MyClass
---@field id integer
local M = Class 'MyClass'

M.showName = 'MyClass'

function M:init()
    self.list = {}
    self.id  = nextID()
end

(Translated by ChatGPT)

In that case the explicitly set fields are not optional unless they're nil-able, or do you meant that it's technically complex to implement?

Anyway it's just an idea if you want to consider a stricter and more consistent type system instead of adding more special keywords. One thing I notice with the language server is that because users (myself included) ask for a lot of orthogonal features, and there is usually one implementer, the annotations start to get more complex and more inconsistencies can arise.

To be clear, I think you're doing a great job managing all this complexity and carefully reviewing suggestions, but I bring up TypeScript because it's a mature language that went through some similar discussions, and the result is a more consistent and strict model that catches many user errors.

I know that I'm probably not thinking of a lot of the other use cases where my idea won't work. Having any kind of @exact annotation would still be extremely helpful. (I prefer ---@class Student <exact> in your example, assuming there will be new kinds of 'attributes' added to classes in the future)

@sumneko
Copy link
Collaborator

sumneko commented May 31, 2023

I can try adding an option that is enabled by default to enable this feature. If many users give feedback that they don't like this feature, I can downgrade it to default disable and provide new annotations to manually mark it.

(Translated by ChatGPT)

@firas-assaad
Copy link
Contributor

I can try adding an option that is enabled by default to enable this feature. If many users give feedback that they don't like this feature, I can downgrade it to default disable and provide new annotations to manually mark it.

(Translated by ChatGPT)

That's a good compromise. I assume it'll still be in a new major version (4?) because I know this will break most of my existing code 😅

You can also make a separate pinned issue to announce this change and solicit more user feedback before doing it, like you did for some features in the past. That way more users will see it coming, at least.

@firas-assaad
Copy link
Contributor

It's also worth mentioning that TypeScript provides a Partial<className> syntax for cases where you want to opt-out of exact types and tell the compiler that all the fields are optional:

interface Person  {
    name:string;
    address:string
    age:number
}
 
let person:Partial<Person>= {}     //No error. All Properties are now optional in person object

@tmillr
Copy link
Contributor

tmillr commented May 31, 2023

I can try adding an option that is enabled by default to enable this feature. If many users give feedback that they don't like this feature, I can downgrade it to default disable and provide new annotations to manually mark it.

(Translated by ChatGPT)

I like that idea. I think this feature is a big (good) one, even if it comes to be an opt-in feature.

@carsakiller
Copy link
Collaborator

@sumneko could it be released as a "preview" version on VS Code? This would allow users to opt in to this version, try it out, update their definitions, and provide feedback on it.

I do agree with the idea of strict by default, as TypeScript does it and it can help prevent a lot of errors – however, it can also add more work, which some may not want:

const myMap = new Map<string, number>();

// Property 'children' does not exist on type 'Map<string, number>'
myMap.children = [];

Need to add .children field somehow

interface ParentMap<K, V> extends Map<K, V> {
  children: Map<K, V>[];
}

const myMap = new Map<string, number>() as ParentMap<string, number>;

myMap.children = [];

@firas-assaad
Copy link
Contributor

No, because many users will declare fields using a mixed approach, for example:

Thinking back about the example and what @carsakiller wrote, my suggestion to assume explicitly set fields (ones defined directly outside of the class annotation) are not optional would weaken the type system by not allowing the Language server to warn about unintentional typos. Preventing the definition of new fields was one of the reasons I started a discussion about such a feature #1602

I'm not sure what's the best approach in this case. Making classes strict by default will enable warnings if you forget to set a property, but allowing a mixed approach to define fields won't catch other kinds of errors. The mixed approach is particularly needed for defining class methods and self variables.

@GiuseppeIII
Copy link

No, because many users will declare fields using a mixed approach, for example:

Thinking back about the example and what @carsakiller wrote, my suggestion to assume explicitly set fields (ones defined directly outside of the class annotation) are not optional would weaken the type system by not allowing the Language server to warn about unintentional typos. Preventing the definition of new fields was one of the reasons I started a discussion about such a feature #1602

I'm not sure what's the best approach in this case. Making classes strict by default will enable warnings if you forget to set a property, but allowing a mixed approach to define fields won't catch other kinds of errors. The mixed approach is particularly needed for defining class methods and self variables.

Just to clarify, it would warn about typos in cases where you are using the class, just not in the case where you are defining the class (which for me is a nice trade-off, but I am sure there are others with differing opinions on this).

@firas-assaad
Copy link
Contributor

firas-assaad commented Jun 14, 2023

It might be able to warn about the use case in this issue (defining a new variable of a type) but it creates a new distinction between the following forms:

---@class Class
---@field y integer
local cls = { y = 1 }
cls.x = true -- no issues, x is now a required part of the `Class` class

---@type Class
local instance = { y = 1, x = false } -- warning if you forget y or x
instance.z = 'hi' -- warning?

That could be a valid compromise if that's technically possible, but it's a new concept for users to learn.

Another question is about self variables. Assuming we're using some OOP library like middleclass.

---@class Class
local Cls= class('Class')

function Cls:initialize(value)
    self.x =value
end

function Cls:func()
   self.y = 1
end

---@type Class
local instance = Cls() -- what does the language server think about `x` and `y`?

There are probably more examples I'm not thinking of. In this example it might be fine to assume that exact type checking is only done if you use the {} table initialization to set variables, so that it would still work with construction function calls.

A different idea is to treat all newly defined variables on the class as optional. That way all the self variables and the class functions are ignored when checking the required fields on an instance. If a user wants to explicitly make a self variable required they'll have to add an explicit @field annotation under the @class one.

With this approach and going back to my first example with cls.x = true on the class, x type should be viewed a non-required boolean. It's not assumed to be nil within the class definition (exactly as it works right now), but users defining new instances of the class don't have to explicitly specify it either.

That means you probably want a new distinction in the language server between being required and being nil-able. One idea is to differentiate the following two forms: ---@field required_var integer? and ---@field optional_var? integer?. Also see this topic about required nullable arguments #2070 (comment)

Finally, if we do end up making fields required by default, then in addition to adding a configuration to disable this behavior, I'd also suggest adding a mechanism for users to tell the language server that they know what they're doing and want to explicitly add new properties to a type. (same way we do with @cast) That would provide a workaround for @carsakiller's example. Alternatively, users can disable that specific diagnostic for a single line.

@sumneko
Copy link
Collaborator

sumneko commented Aug 11, 2023

I can do this first:

---@class Class
local m = {}

m.xx = 1 -- OK

---@type Class
local m

m.xx = 1 -- OK
m.yy = 1 -- Warning

I'm not checking the assignments inside class declarations temporarily because I still haven't thought of a good way to differentiate this case:

---@class Class
local m = {}

function m:init()
    m.xx = true -- should OK
end

function m:doSomething()
    m.xxx = false -- missspell, should warning
end

@lewis6991
Copy link
Contributor

lewis6991 commented Aug 11, 2023

How about something like a @sealed annotation? Then this can be opt-in for specific classes.

---@class Class
local m = {}

m.xx = 1 -- OK

---@type Class
local m

m.xx = 1 -- OK
m.yy = 1 -- OK
---@class Class
---@sealed
---@field xx integer
local m = {}

m.xx = 1 -- OK

---@type Class
local m

m.xx = 1 -- OK
m.yy = 1 -- Warning

@sealed meaning the class cannot have new members dynamically added to it.

The downside of this would mean all methods would need to be declared up front.

@sumneko
Copy link
Collaborator

sumneko commented Aug 11, 2023

The downside of this would mean all methods would need to be declared up front.

I cannot allow injecting fields from type references; if I did, the cost of correctly collecting these fields would be unacceptable. Therefore, you must use ---@Class to inject fields.

@lewis6991
Copy link
Contributor

lewis6991 commented Aug 11, 2023

Ah sorry, I should have changed the example so m isn't aliased.

What I meant was this.

---@class Class
local m = {}

m.xx = 1 -- OK
m.yy = 1 -- OK
---@class Class
---@sealed
---@field xx integer
local m = {}

m.xx = 1 -- OK
m.yy = 1 -- Warning

However, I think I now understand your suggestion better. Are you saying any fields defined on the initial @class variable become fields:

---@class Class
local m = {} -- Anything assigned to `m` is added as a field of `Class`

m.xx = 1 -- OK: adds `xx` as a field to `Class`

---@type Class
local m2 -- `m2` is an instance of `Class` with the fields: `xx`.

m2.xx = 1 -- OK
m2.yy = 1 -- Warning

I think this is a good idea since it doesn't require any additional annotation syntax.

@lewis6991
Copy link
Contributor

lewis6991 commented Aug 11, 2023

For the other case how about:

---@class Class
local m = {}

function m:init()
    m.xx = true -- OK: adding field `xx` to `m`
    self.xx = true -- OK: `xx` is a field of `m`

    self.yy = true -- OK: `yy` is a field of `m`
    m.yy = true -- OK: adding field `yy` to `m`
end

function m:doSomething()
    self.xxx = false -- WARNING: m does not have field `xxx`
end

Basically we need to check all fields that can ever be assigned m even if done dynamically.

@sumneko
Copy link
Collaborator

sumneko commented Aug 11, 2023

For the other case how about:

---@class Class
local m = {}

function m:init()
    m.xx = true -- OK: adding field `xx` to `m`
    self.xx = true -- OK: `xx` is a field of `m`

    self.yy = true -- OK: `yy` is a field of `m`
    m.yy = true -- OK: adding field `yy` to `m`
end

function m:doSomething()
    self.xxx = false -- WARNING: m does not have field `xxx`
end

Basically we need to check all fields that can ever be assigned m even if done dynamically.

This does not conform to the writing habits of most people

@sumneko
Copy link
Collaborator

sumneko commented Aug 11, 2023

@carsakiller any suggests for name and syntax of exact or sealed ?

@lewis6991
Copy link
Contributor

Can I ask an example(s) of typical writing habits you wish to support?

My suggestion works up your suggestion by defining a base table that acts as a definition of the class.

@sumneko
Copy link
Collaborator

sumneko commented Aug 11, 2023

Can I ask an example(s) of typical writing habits you wish to support?

My suggestion works up your suggestion by defining a base table that acts as a definition of the class.

---@class Class
---@field a number -- priority 1
local m = {
    b = 2, -- priority 2
}

m.c = 3 -- priority 3

function m:init()
    self.d = 4 -- priority 4
end

@GiuseppeIII
Copy link

Just to throw my two cents in here.

The most useful version of an @strict for me would be the similar to as described by @Mayron where all of the fields have to be defined up-front. And using any non- or incorrectly defined fields would cause an error. This is because the times I generally want to use such an annotation is when I am making an 'inteface' for function arguments and less so when I am defining an actual class.

The other form of the annotation as descirbed by @lewis6991 seems more useful when trying to 'lock' instances of classes from implementing additional methods or variables.

I think this thread has generally been around the @strict approach, but I can see seperate @strict and @locked class annotations being useful.

sumneko added a commit that referenced this issue Aug 14, 2023
@sumneko
Copy link
Collaborator

sumneko commented Aug 14, 2023

I personally prefer to put attribute and objects together. Originally, referring to Lua's own variable attribute syntax local x <close> = file, using angle brackets would be better, but it would indeed conflict with future generic syntax. Perhaps other forms of brackets will be used, such as:

  • ---@class Class {exact}
  • ---@class Class (exact)

Or using symbols:

  • ---@class Class $exact

In addition, by the way, I plan to make the attribute of field also support the corresponding syntax, such as:

  • ---@field (private) x number
  • ---@field $private x number

@lewis6991
Copy link
Contributor

In that case. In order to be consistent with @field, would it make sense for these modifiers strictly to follow the annotation?

-- @class $exact myclass
-- @field $private a integer

I must admit I'm not a fan of any of the suggestions thus far. $ and{} just doesn't sit well with me but maybe it'll grow.

Just to add another one to the mix.

-- @class (exact) myclass
-- @field (private) myfield integer

And generally:

-- @annotation (mod1) (mod2) ... (modn) name

-- or is this better?

-- @annotation (mod1,mod2,...) name

Whitespace insensitive.

sumneko added a commit that referenced this issue Aug 14, 2023
@sumneko
Copy link
Collaborator

sumneko commented Aug 14, 2023

I'd like this now:

---@class (exact, attr2, attr3) myclass
---@field (private, attr2) myfield integer

@Mayron
Copy link

Mayron commented Aug 14, 2023

exact relies (at least in this case) on the @Class annotation, so this doesn't make much sense.

  • @field requires a @class
  • @operator requires a @class
  • @async requires a function
  • @generic requires a function
  • @nodiscard requires a function
  • @overload requires a function
  • @package, @private and @protected require a function
  • @param requires a function
  • @returns requires a function

I am referring to annotation using LS declarations only. Giving examples of using annotation that requires Lua syntax isn't relevant to my point.

The first 2 of your examples are the only relevant ones and I agree with is the @field / @operator requires a @class examples, because both of these can be used to create LS rules without the need for Lua. The rest of your examples that mention using annotation with a Lua function is pretty obvious and less prone to error.

So, yes I agree now that @exact can work by itself and I have no problem with that. I don't honestly mind whichever direction is agreed on as long as the feature is added.

@Mayron
Copy link

Mayron commented Aug 14, 2023

  • ---@field (private) x number

I like this a lot, because it opens the doors to many other features, including the use of a readonly modifier like some other people (including myself) have suggested ;)

(for example, if myValue was declared as readonly, then tbl.myValue = 5 would not be allowed if marked as readonly).

EDIT: Personally, I prefer the $ syntax and add it before the identifier like the example given (-- @field $private a integer), but happy with whatever the community decides on.

@sumneko
Copy link
Collaborator

sumneko commented Aug 14, 2023

How about tail:

---@class myclass (exact, attr2, attr3)
---@field x integer (private, attr2)
---@field y integer (private, attr2)

This syntax can avoid alignment issues between fields.

EDIT:
It may be confused with comments.

@lewis6991
Copy link
Contributor

Personally I prefer the former. If () becomes required then that makes the grammar cleaner and easier to parse.

Something like:

"@class" optional("(" commalist(modifier) ")") name optional("<" commalist(name) ">") optional(":" type)
"@field" optional("(" commalist(modifiers) ")") choice(name, "[" type "]") type optional(description)

@sumneko
Copy link
Collaborator

sumneko commented Aug 22, 2023

---@class (exact) Class
---@field x number
local m = {
    x = 1, -- OK
    y = 2, -- Warning
}

m.x = 1 -- OK
m.y = 2 -- Warning

function m:init() -- should this OK?
    self.x = 1 -- OK
    self.y = 2 -- Warning
end

@lewis6991
Copy link
Contributor

lewis6991 commented Aug 22, 2023

I think all the warnings in that snippet shouldn't be warnings as m defines the class.

At the very least m.y and m.init should be consistent.

@sumneko
Copy link
Collaborator

sumneko commented Aug 22, 2023

I think all the warnings in that snippet shouldn't be warnings as m defines the class.

We should not treat a particular name specially. In code I've seen, users may use names such as m, M, mt, _M, and so on.

@lewis6991
Copy link
Contributor

lewis6991 commented Aug 22, 2023

Yes, they should all work. Any name should work.

The point is that it is the table attached to the class.

--- @class (exact) c
local obj = {
-- anything defined in here is a defined field of c
}

--- @type c
local inst = {
-- has fields defined in obj
}

--- @class (exact) d

local other = {
-- this does not define d, since it doesn't come directly after the class
}

This will allow users not needing to define fields multiple times. The methods can be defined once.

@firas-assaad
Copy link
Contributor

---@class (exact) Class
---@field x number
local m = {
    x = 1, -- OK
    y = 2, -- Warning
}

m.x = 1 -- OK
m.y = 2 -- Warning

function m:init() -- should this OK?
    self.x = 1 -- OK
    self.y = 2 -- Warning
end

Looks good to me. If there's a way to be smart and not show the first warning (defining a field directly under @class) (as @lewis6991 suggests) then that would be nice.

For functions like init, I guess it's OK if you allow it. Even if you don't, I actually don't mind having to define all the fields and even function in advance even if it's duplicated work. The benefit of having exact/strict classes would already bring a lot of benefits even if the implementation isn't perfect.

I do think the second and third warnings in your example (m.y = 2 and self.y = 2) are appropriate and are one of the main use cases behind these feature requests.

When it comes to function it's trickier, because it's a more common usage pattern than assigning normal variables. Maybe if I can write something like this:

---@class (exact) Class
---@field x number
---@field (method) init function -- or maybe `@field init method ` or even `@method init`
local m = {
    x = 1, -- OK
    y = 2, -- OK because it follows the @class definition
}

m.x = 1 -- OK
m.y = 2 -- OK because it was defined in the first assignment of m
m.z = 2 -- Warning because we never saw it before

function m:init() -- OK because `init` was explicitly defined as a method @field
    self.x = 1 -- OK
    self.y = 2 -- OK
    self.z = function() end -- Warning
end

function m:process() end -- Warning because it's not part of the @class definition

The idea is that you don't have to explicitly define the signature of a method like init. Instead, you tell LuaLS that it is a method that will be defined later. All such methods must be part of the @class annotations or you get warnings when trying to define them. That way you can't introduce new (wrong) method names by mistake (or define methods on C userdata, for example)

You don't really need a new syntax for annotating methods. This can work with @field init function too, as long as LuaLS is able to use the actual function signature (where m:init is defined) for completion and type checking instead of the generic function. I thought adding a keyword or attribute like method makes the developer's intent clearer, but what matters if that the user can write the name of these methods without having to explicitly write ---@field init fun(Class:self), especially if the method signature is complex and has overloads.

Anyway, it's just an idea. I think even with just your proposal it'd already be a very useful feature. After all, this is an opt-in feature with (exact), so it's a good idea to make it more strict and people who don't want this strictness don't have to use it.

@lewis6991
Copy link
Contributor

lewis6991 commented Aug 22, 2023

Ideally we should treat all fields consistently, whether they are functions or not:

---@class (exact) Class
---@field x number
local m = {
    x = 1, -- OK
    y = 2, -- OK because it is assigned into `m` which is the table which follows the class definition.
}

m.x = 1 -- OK
m.y = 2 -- OK because it is assigned to m
m.z = 2 -- OK becuase it is assigned to m

function m:init() -- Ok becuase it is assigned to m
    self.x = 1 -- OK
    self.y = 2 -- OK
    self.z = function() end -- Warning as `z` should be an integer
end

function m:process() end -- Ok

As opposed to a class instance:

--- @type Class
local m2 = {
  x = 1, -- OK
  a = 2, -- Warning as `a` is not defined in `m` or `Class`
}

function m2:init() -- Ok becuase it is assigned to m
    self.x = 1 -- OK
    self.y = 2 -- OK
    self.b = 3 -- Warning: `b` is not a member of `Class` or `m`
end

function m2:process() end -- OK
function m2:process2() end -- Warning

If the user wants strict exact semantics, then do not define a table after the @class definition.

---@class (exact) Class
---@field x number
---@field init function

-- No table after `Class` so no more fields can be added.

--- @type Class
local m = {
    x = 1, -- OK
    y = 2, -- Warning
}

m.x = 1 -- OK
m.y = 2 -- Warning
m.z = 2 -- Warning 

function m:init() -- Ok
    self.x = 1 -- OK
    self.y = 2 -- Warning
    self.z = function() end -- Warning
end

function m:process() end -- Warning because it's not part of the @class definition

@firas-assaad
Copy link
Contributor

firas-assaad commented Aug 22, 2023

Ideally we should treat all fields consistently, whether they are functions or not:

I'm fine with this too but I'm not sure if the language server is actually able to track local variables at that level, especially if you consider aliases or returning m from a module and other use cases.

Edit: I'd argue that defining things on self inside functions isn't really part of the class definition and your approach loses some valuable warnings by treating it the same as m

@firas-assaad
Copy link
Contributor

self.z = function() end -- Warning as z should be an integer

I also don't get why this z has preference over m.z. What if you defined m.z = 2 after the function definition?

@lewis6991
Copy link
Contributor

lewis6991 commented Aug 22, 2023

self.z = function() end -- Warning as z should be an integer

I also don't get why this z has preference over m.z. What if you defined m.z = 2 after the function definition?

You're right. That one isn't so straight forward, but self is usually an instance. It doesn't necessarily point to m, though it technically could.

What if you defined m.z = 2 after the function definition?

LuaLS would need to run a static analysis, so it shouldn't matter where the assignment happens, just that it does.

@lewis6991
Copy link
Contributor

Edit: I'd argue that defining things on self inside functions isn't really part of the class definition and your approach loses some valuable warnings by treating it the same as m

My approach doesn't treat self as part of the class definition.

@firas-assaad
Copy link
Contributor

firas-assaad commented Aug 22, 2023

My approach doesn't treat self as part of the class definition.

Right. I misread it. So how would you handle say self.w = 3 inside the init function?

@lewis6991
Copy link
Contributor

lewis6991 commented Aug 22, 2023

Right. I misread it. So how would you handle say self.w = 3 inside the init function?

Warning. self should be treated as an instance.

See #1990 (comment)

sumneko added a commit that referenced this issue Aug 23, 2023
@firas-assaad
Copy link
Contributor

firas-assaad commented Aug 25, 2023

@sumneko Thank you so much for implementing this. It already helped me spot some issues in my code.

Is it possible to emit a warning when a table is defined here (when setting w = 2):

---@class (exact) Class
---@field x integer

---@type Class
local y = {
    x = 1, -- Fine
    w = 2, -- Should warn, but doesn't?
}

y.x = 2 -- Fine
y.z = 4 -- Warning! Works as expected

Also for nested cases:

---@class Another
---@field c Class

---@type Another
local a = {
    c = {
        x = 1,
        w = 4, -- Currently there are no warnings
    }
}

Let me know if you prefer that I open another issue for this.

Edit: the issue is probably the same as this one: #2288

@lewis6991
Copy link
Contributor

lewis6991 commented Aug 25, 2023

Am I right in guessing that in c2018e0 methods never report a warning?

@bavalpey
Copy link
Contributor

Is there a reason this is still open considering we have (exact)?

@mikavilpas
Copy link

@bavalpey I think this should be open. The following should report an error but doesn't

---@class (exact) Student
---@field study_level integer

---@type Student
local new_student = {
    study_level = 0,
    tobacco_level = 1, -- This should be error.
}

@MillhioreBT
Copy link

@bavalpey I think this should be open. The following should report an error but doesn't

---@class (exact) Student
---@field study_level integer

---@type Student
local new_student = {
    study_level = 0,
    tobacco_level = 1, -- This should be error.
}
---@class (exact) Student
---@field study_level integer
local new_student = {
	study_level = 0,
	other = 0 -- ERROR
}

---@type Student
local i = {
	study_level = 1,
	other = 0 -- NO ERROR ??
}

i.other = 0 -- ERROR

In these cases it works, however in the case of ---@type it seems not. It's weird but it makes sense that it would work, or maybe not? I hope some expert on the subject has something to say.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests