mesonbuild / meson

The Meson Build System
http://mesonbuild.com
Apache License 2.0
5.35k stars 1.53k forks source link

Differentiate global variables from local variables #2607

Open ghost opened 6 years ago

ghost commented 6 years ago

So with Meson all the variables are accessible in all meson.build files. For a big project, this can become a maintenance problem.

Normally the variables are immutable, so writing by mistake the same variable name in two different meson.build files should trigger an error (but this is currently not the case, see https://github.com/mesonbuild/meson/issues/2606 ).

It would be better to be able to differentiate local variables (those used in only one meson.build file) from global variables (those used in several meson.build files). It is possible to use a convention when naming those variables, but it would be better to have support from the type system.

ghost commented 6 years ago

Normally the variables are immutable, so writing by mistake the same variable name in two different meson.build files should trigger an error (but this is currently not the case, see #2606 ).

OK so since this is not a bug, it makes the whole thing much more fragile.

I've adopted this convention now:

liugang commented 6 years ago

support local keyword before local variable maybe a good choice

do this is a good idea, but it will broken lots of existed meson projects.

Local variables in lower_case.
Global variables in UPPER_CASE.
ghost commented 6 years ago

To not break the Meson API, there could also be a new setting in the project() command to enable the feature. And this could become the default for Meson version 2.

That way the feature can be implemented the other way around: local variables by default, and for a variable to be global, a keyword must be added, like "global" or "export". I find it more natural like this.

nirbheek commented 6 years ago

There is no such thing as 'global' or 'local' scope in Meson since the concept of scopes does not exist, and does not make sense since there are no functions. The ability to place build files in subdirs is provided for ease of use, and it is essential for variables to be accessible between build files in different subdirs. For example, you create a library in one directory and use it in your tools, tests, examples, and other library directories.

However, it is a real problem that people can clobber variables without meaning to. I think we can add a qualifier like const that causes an error when variables are reassigned. Alternatively, we can warn by default on clobbers and have a qualifier like var that suppresses it.

I like the const way though because it makes it very explicit that some variables are special and that their value will be reused elsewhere, say via a subproject or in other subdirs.

ghost commented 6 years ago

In Object-Oriented Programming, there is this concept called encapsulation, also known as information hiding. A code following that practice is easier to reason about, because it is possible to understand a piece of code without the need to understand all the rest. A class exports a public interface and hides its implementation details, so when using that class in another part of the codebase, we can just look at its public interface to know how to use it and what features it provides.

For a meson.build file, its public interface would just be its list of global variables, i.e. the variables that are meant to be used in other meson.build files.

If you have read any sort of programming best-practices guide, you should know that global variables should be avoided if possible. In Meson, all variables are global… Don't you see anything wrong with that? In order to understand a meson.build file, you must potentially look at all the other meson.build files to know where is the relevant variable assignment (with mutable variables it makes things worse since a variable can be re-assigned in any other meson.build file). So it makes things harder to reason about.

Even if the type system doesn't enforce it, it is useful to distinguish global variables from local variables when reading the code. With my above convention with global variables in UPPER_CASE, I directly know if a variable is meant to be used in other meson.build files. In C, you can look at a *.h file to know what the "class" exports in its public interface. With Autotools the "global variables" are exported explicitly with the AC_SUBST macro. Currently in most projects using Meson as its build system, when reading a meson.build file there is no way to know the list of variables that are meant to be used in other meson.build files, i.e. its public interface.

Something that would be useful is preventing a variable from being re-assigned in another meson.build file. So a variable would be defined in a certain meson.build file, and all other meson.build files could use that variable, but not assign it a different value. A little the same as AC_SUBST.

nirbheek commented 6 years ago

Meson's build file language is not a programming language, and build files in general have totally different use-cases compared to programming languages. It is more useful to look at specific use-cases than trying to reason by analogy.

Sometimes it can indeed be difficult to figure out where a variable is defined if it's used across subdir(), but the fact that it works like that is also a useful feature that is used by all non-trivial build files that I have seen.

A good rule of thumb is to define the variable in the parent build file of whichever build files use it, unless it points to a target or something else that is entirely contained within a subdir. In general, I think this problem is better solved via your IDE.

I also don't agree with the Autotools comparison at all because AC_SUBST is about autoconf substituting variables in .in files, so the equivalent in Meson is actually configure_file() + configuration_data().

Something that would be useful is preventing a variable from being re-assigned in another meson.build file

I agree, that is a user error that we can easily design to avoid, which is why I suggested const.

jpakkane commented 6 years ago

When this was originally designed there was a very difficult balancing act between ease of use and security. The current approach was chosen due to objects being immutable and most projects needing to have variables visible. The biggest isolator is subprojects, where variables are fully separated and can only be accessed in limited ways and explicitly.

Like most things in life this is not a perfect solution but a compromise between many different competing requirements.

jibsen commented 6 years ago

One workaround would be to prefix variables that you wish were local with the subdir they are in. It looks like the glib meson build system sometimes does this.

With all variables being global and objects being immutable, there will be a proliferation in variable names. Perhaps if there were a switch to get warnings about clobbering, especially across subdirs?

I am personally not a fan of using all caps names, that could quickly make meson build files look like CMake.

ghost commented 6 years ago

Meson's build file language is not a programming language

There is:

Code is code. It is important to write clean and maintainable code. It is no different for the build system.

TingPing commented 6 years ago

@jibsen I think that is just best practice in general and mostly avoids any accidental conflicts.

volo-zyko commented 6 years ago

that could quickly make meson build files look like CMake.

It's not that easy. CMake is 18 years effort. :)

Seriously, does Python code look like CMake? They also have a convention about all caps global variable names. Separating local and global variables with a different naming scheme is a good thing. It makes the code more maintainable/readable. And this could be just a convention. That's my opinion.

So, what's the decision for this ticket? I'm curious because I'm considering meson for using in a big proprietary project and this issue is important to me.

kugel- commented 5 years ago

I too think local variables are crucial for any bigger project.

Ericson2314 commented 5 years ago

FWIW I would prefer the restrictions of something like https://github.com/dhall-lang/dhall-lang rather than no functions/scope to keep the language decidable. Some day...

rulatir commented 4 years ago

Literally everyone:

It would be better to be able to differentiate local variables (those used in only one meson.build file) from global variables (those used in several meson.build files).

Meson authors:

God forbid! If we allow this, people will start using it to circumvent Meson's opinionatedness!

ceztko commented 3 years ago

Not only I support the idea of having local variables but I suggest that local variables should have been the default from the beginning, with a global qualifier to make them global. CMake devs understood correctly the importance of scoping, but their final design is also not fantastic:

Meson scoping is only restricted to dependencies and/or subprojects. Within the project you can clutter the namespace and introduce encapsulation problems (eg. reassigning variables with wrong content). The minimum fix that keeps compatibility would be introduce scopes, dictated by directories or by other means, and a local qualifier for variables. A radical breaking change that makes all variables local by default and allow to qualify them with global would also be appreciated on my side.

mensinda commented 3 years ago

local variables should have been the default from the beginning, with a global qualifier to make them global.

Unfortunately, we do not have a time machine...

A radical breaking change that makes all variables local by default and allow to qualify them with global would also be appreciated on my side.

This won't happen since this would break almost all projects that use multiple meson.build files.

However, here are some of my ideas that could work (without breaking backward compatibility):

ceztko commented 3 years ago
  • add const (should be fairly easy)
  • warn when variables are reassigned with a different type
  • add scoping

    • local and global modifiers
    • the default is global for all variables (may be changed to local with a project() parameter)
    • optionally: explicit scopes with someting like block, endblock

All of this would be fantastic (block, endblock is actually a feature that I wanted to suggest also to CMake, as a complement to named function). I also thought the same interventions but I didn't say to this extent because it's fairly too easy only suggesting when there is lot of work involved. You didn't mention implicit directories scoping by I think you also meant that.

MikuChan03 commented 3 years ago

Ok, when will work on this commence?

Ericson2314 commented 3 years ago

global is not a good default. local as a keepweird smells like bash, and i don't want to smell like bash.

IMO pragma at the top of a file or similar, and just clean break with the old stuff. Dynamic scoping is terrible.

(To be clear, while I am a member of the mesonbuild org, I do not have the authority to make things happen according to these opinions of mine, so don't ask me to.)

dcbaker commented 3 years ago

I think it's worth fixing, but it's going to be a ton of work. If someone wanted to do all that work, they could probably just add a new variable syntax:

var: mut global = 'foo'

That should be syntactically different enough from the existing var = 'foo', that the old syntax can remain global and mutable, without any changes, and could simply be deprecated out at some point.

I propose if someone's going to do the work that variables be immutable and local by default. But please, let's do the smart thing and not put the variable attributes on the left, but on the right.

MikuChan03 commented 3 years ago

Ok, when will work on this commence?

ceztko commented 3 years ago

global is not a good default. local as a keepweird smells like bash, and i don't want to smell like bash.

Agree 100%. When there's a design error like this, it's just better to fix it and not keep the stain forever. After all, meson it's not a programming language and breaking changes can be fully anticipated with warnings in earlier releases. CMake for example anticipated several breaking changes forcing users to change their code or set policies[1] to avoid warnings. As a result of this CMake today is in a good/decent shape. Git also did a similar move when it changed the semantics for the push command.

[1] https://cmake.org/cmake/help/latest/command/cmake_policy.html

dcbaker commented 3 years ago

My experience with cmake policies is that they're a disaster. Everyone wants to turn the annoying warnings off, and ends up breaking old versions. Meson had taken an approach of deprecating things at a specific version, if you set your minimum version >= the deprecated version we warn you, otherwise we don't. That keeps old projects running without a bunch of warnings that they can't fix. I'd really prefer to see a different syntax so we can keep that strategy.

ceztko commented 3 years ago

My experience with cmake policies is that they're a disaster.

Policies are just an instrument to communicate a behavior change. Depending on how they were scheduled/handled, and they ways offered to silence the warnings, they could offer a good (or bad) way to inform a bigger audience of an incoming change, in some cases without requiring minimum version upgrade. I had a couple of experiences with policies and they were positive, but I understand this may have not worked well in all the cases.

if you set your minimum version >= the deprecated version we warn you

Since meson projects create boundaries you could also use minimum version to decide a different behavior.

Ericson2314 commented 3 years ago

Since meson projects create boundaries you could also use minimum version to decide a different behavior.

Yes we should get rid of >= for the meson version. Rather you specify = or no symbol at all, which doesn't mean "must use the meson version", but rather means "I am using the interface as of this version". Never versions of meson are of course free to support the older interface, and one can warn about stuff this both too old or too new.

c.f. https://cabal.readthedocs.io/en/3.4/cabal-package.html?highlight=version#pkg-field-cabal-version & https://github.com/haskell/cabal/issues/4899, when the Cabal, the Haskell build tool, was in this exact same situation.

eli-schwartz commented 3 years ago

The current use of >= already is interpreted to mean warn about stuff that's too new, and hard error if your meson version is too old. I don't see how letting people run meson with warnings that "this will definitely 100% not work, your meson is too old, but go try it anyway" is anything but a reduction in functionality.

Furthermore, use of

project('foo', 'c', meson_version: '0.54.0')

according to your proposal, is already reserved by the current syntax to mean "hard error for both too new versions and too old versions". So your proposal is completely dead in the water; meson is not going to change the meaning of this and break current projects using this in the wild (and let's not kid ourselves that no one is using this and expecting it to do just that).

I completely don't understand why you think any aspect of this current ticket implies, requires, or suggests, getting rid of two bytes for inexplicable reasons. The former suggestion to just use that explicit version as the inferred compatibility level, is perfectly adequate for the needs of this ticket.

Tachi107 commented 3 years ago

I think that at least const should be added in one of the next releases. Sure, it might not be as cool as C++20 modules support, but the faster it gets added the less error-prone meson.build files will be.

I know that asking for something and expecting it to be immediately implemented by a group of volunteers is terrible thing to do, but unfortunately I don't know a thing about Python and can't really help :/

punytroll commented 2 years ago
* add `const` (should be fairly easy)

* warn when variables are reassigned with a different type

* add scoping

  * `local` and `global` modifiers
  * the default is `global` for all variables (may be changed to `local` with a `project()` parameter)
  * optionally: explicit scopes with someting like `block`, `endblock`

I've stumbled over this problem myself, and I have implemented a three-liner which provides a warning to the user in case a variable is reassigned with a different type. (point 2 on the list)

Granted, this issue asks for more but I thought I'd start small.

Is there still any interest in this?

Tachi107 commented 2 years ago

I'd personally really like having Meson at least warn me when I accidentally reassign variables, but maybe there's someone out there that reassigns variables consciously, so this would probably make them furious... I guess that this should be discussed a bit more

tristan957 commented 2 years ago

that seems like the job of a linter. muon just got a static analysis feature.

punytroll commented 2 years ago

I'd personally really like having Meson at least warn me when I accidentally reassign variables, but maybe there's someone out there that reassigns variables consciously, so this would probably make them furious... I guess that this should be discussed a bit more

While running the unit tests, I saw some of those use the same variable for multiple executable() calls. If one of those calls returns a disabler, then that is different type than the expected executable. (i.e. test case/common/2 cpp) I'm not sure whether this is a quirk of that particular test, or a common pattern ...

Also, this is only about accidental type changing reassignment. Maybe this gives an even greater false sense of security?!

inigomartinez commented 2 years ago

I'd personally really like having Meson at least warn me when I accidentally reassign variables, but maybe there's someone out there that reassigns variables consciously, so this would probably make them furious... I guess that this should be discussed a bit more

I am one of those. I usually reasing variables in different directories with same meanings, to clearly show what they try to do.

dotnwat commented 2 years ago

huge +1 from me. local foo = 3 limited to scope like a foreach, but especially to a single file.

davidgiven commented 2 years ago

I'd like this as well. Trying to keep track of temporary variables across the entire build system is painful.

annacrombie commented 1 year ago

I'd like to add that muon analyze supports a related feature with -Wreassign-to-conflicting-type. It can be quite noisy though since some short variable names are often reused, e.g. loop iterator variables.

burnpanck commented 9 months ago

I'd like to have local variables too. Encapsulation is great - no matter if meson is a programming language or not. I use variables to avoid repetition in meson files, and this is a purely local use-case. In that case, I don't want to prefix every variable with a large prefix just to avoid risking overwriting something relevant from an outer "scope" (I know, meson doesn't have scopes - but that is how I reason over things).

dcbaker commented 9 months ago

What I've seen in this thread are requests for:

I'd prefer a separate conversation for const/immutable variables, as it's really tangential.

Personally, and I think in general, we would be opened to having some sort of variable scoping. For backwards compatibility reasons it would have to be opt in, (I'm not placing a syntax requirement) foo: local = 'bar', so that projects do not break. If we were to do this, I would assert that having local as the default in meson 2.0 makes sense. as such adding foo: global = 'bar' at the same time makes sense, so that projects can be ready for the local by default.

Among other things I would expect from a local variable:

I haven't seen any other maintainers actually object to local, so I would say that if you're really interested, a proposal of what the syntax and rules of local variables would actually be would be the logical next step.

eli-schwartz commented 9 months ago

I haven't seen any other maintainers actually object to local

I do not have strong feelings about what the solution should be, but I definitely agree that this enhancement request is a good idea and we should implement it somehow.

xclaesse commented 9 months ago

I'm personally in favor of having some syntax for local variables. I'm not sure about const because almost all our objects are immutable already, or immutable after use, I assume that would mean we cannot assign over the existing variable?

About Meson2 default, let's cross that bridge when/if we get there, but IMHO we cannot change this forever. Adding such huge implication changes in our syntax will just mean large projects are going to keep Meson1 for decades. I'm personally much more in favor of incremental small breaking changes after deprecation period rather than big "2.0" breaks. See how gtk3 is still being used in so many places...

dcbaker commented 9 months ago

I'm not sure about const because almost all our objects are immutable already, or immutable after use, I assume that would mean we cannot assign over the existing variable?

I think what is really meant here is more like Java's final keyword, once a variable is assigned it cannot be re-assigned

I'm looking at what Python did (later, in the more successful era of building software that worked on both python 2.x and 3.x), where they provided ways to write software that worked correct on both. so Ideally, in meson 1.x you could have:

x: local = 'foo'
dep: global = declare_dependency()

and that would work for both. But we can argue that out as we get closer to the 2.0 era, for now I would absolutely want both a global and a local specifier

tristan957 commented 9 months ago

Another syntax format could be

let x = 'foo' # local
var y = declare_dependency() #global

But that is JavaScript-minded, so might not be entirely understandable at first glance.

Alternatively:

local x = 'foo'
global y = declare_dependency()

This last one kind of takes inspiration from bash which has a local keyword. global could also be substituted for export in the previous example.

Though I wonder if either of these proposals are harder to add retroactively as opposed to the suffix that you are proposing, which is similar to what Python did with typing.

dcbaker commented 9 months ago

There are reasons that most modern languages (Rust, Zig, Swift, Python, TypeScript) have the [let] identifier[: modifier [modifier]*] = value syntax, which is simpler to write a lexer/parser for, due to how much look ahead you need to do if there's multiple modifiers. imagine somthing like: x: final global = 'foo' vs final global x = 'foo' In the first case you know immediately that you have a variable x, and that some number of modifiers will follow until the =, while in the second case you have to read until the = to know that you're dealing with two modifiers to the variable x

burnpanck commented 9 months ago

I believe any kind of language supposed to be written by humans should be optimised towards the human parser, not the compiler's lexer/parser. That said, at least in this case, I think both lead to the same result: The modifiers contain much less information than the variable name itself, so the variable name should have priority for the easiest to spot location.

jibsen commented 9 months ago

Is there a need for global if we had local? For instance in Lua all variables are global unless declared local.

dcbaker commented 8 months ago

@jibsen ideally variables would be local unless declared global, and if we wanted to swap the default from global (which is what we would need for backwards compatibility) in a later major version, having an explicit global modifier would allow one to write a meson.build file that would work with both by explicitly tagging everything.

jibsen commented 8 months ago

Ah, I see -- I missed the point about possibly changing the default, thank you.

While I am a big fan of backwards compatibility, my immediate reaction is perhaps slight unease at the idea of adding a keyword that presently does nothing for the sake of a possible future change.

Perhaps Meson 2.0 could have something similar to the policy system of CMake, where if the Meson file requests a minimum version below 2.0, the default reverts to global by default. That policy could then be deprecated at some later point when enough people have updated to a minimum of 2.0.

I imagine we could write a modernizer for Meson 2.0, similar to the ones Clang has, that goes through Meson files and adds/removes global and local as appropriate (along with other breaking changes). People could then run that before updating the minimum required version of their Meson files to 2.0.

I realize this shifts the burden onto the Meson developers.

It is great that this is being discussed again, it would be lovely to have support for local in some way.

dcbaker commented 8 months ago

We have consciously decided not to have a policy system for a number of reasons. One is that CMake's policy system generates a lot of noise that leads to people helpfully sending patches that break older versions of cmake. The second is that it makes the implementation very complicated and increases our need for tests. We already struggle with missing coverage and very long test runs.

eli-schwartz commented 8 months ago

While I am a big fan of backwards compatibility, my immediate reaction is perhaps slight unease at the idea of adding a keyword that presently does nothing for the sake of a possible future change.

What are your thoughts on users who add kwargs to various function calls in their meson.build, such as required: true when it's already the default for dependency/find_library?

There is a school of thought that says "explicit is better than implicit". I've seen rather a lot of meson.build files which follow that. I wouldn't say it's a majority...

dcbaker commented 8 months ago

Sort of a tangent, but I've thought it would be nice to be able to mark variables that we plan to export, ie those that we consider to be public API to subprojects. This comes up because Mesa uses a lot of internal helper dependencies (via declare_dependency) that we don't expect to be used externally. As a followup I might propose an export modifier like:

x: export global = declare_dependency(...)

That would be simpler from a backwards compat point of view since we could use our normal feature mechanisms, since it would remain a warning until 2.x anyway. And actually, as we approached the 2.x era we could even use our normal feature mechanisms in 1.x for tagged variables

xclaesse commented 8 months ago

@dcbaker ideally those variables should be passed through declare_dependency(), and same variable should be set in pkgconfig as well. That way parent project can work indifferently whether it's internal or external dependency. But I guess that's not always possible any specific use case in mind?