Big update to flesh out const instance methods

[llvm-beanz]

I started digging a bit into this in DXC and identified a lot of cross-cutting issues that we need to consider in this.

This change updates the proposal to flesh out the behavior and related issues around HLSL const-correctness.

This does identify some source-breaking changes that this would introduce as well as some behavior changes that I think we should introduce in the HLSL built-in types.

[ChristianReinbold]

After this PR has been inactive for almost a year now: Are the plans to bring const qualifiers to instance methods still valid? At the moment, I have to compile a lot of shared code (C++ & HLSL) by removing the const keyword with the preprocessor (#define const) whenever dxc is used as compiler. This comes with some obvious drawback, the biggest afaic being that static const variables do not work anymore. (const-correctness is still guaranteed in the C++-pass)

For the time being, I consider hacking in the tok::kw_const case in Parser::ParseTypeQualifierListOpt such that dxc accepts const qualifiers without ending up with a diagnostic. This approach seems to work for me. This PR however suggests that there is more work to do for proper const-qualifier support.

If I know that at some point in the future const-methods will be supported, I would be more inclined to accept the hack mentioned above as a temporary measure.

[llvm-beanz]

@ChristianReinbold, thank you for your interest in the HLSL language.

After this PR has been inactive for almost a year now: Are the plans to bring const qualifiers to instance methods still valid?

Our team is very small and our focus has been on Shader Model 6.8 rather than HLSL 202x over the last year. Which may seem like there is no progress here, but we are still planning this as a likely feature for the next HLSL language version.

At the moment, I have to compile a lot of shared code (C++ & HLSL)...

This is going to be a really difficult thing to do with HLSL as the language exists today. There's a lot of differences between HLSL and C++, so this use case isn't something we actively support with HLSL today (although we are working to support it in the future).

(const-correctness is still guaranteed in the C++-pass)

I assume this is your justification for why the hack you describe below is "ok", but your specific use case isn't the same as others so it isn't something we can bank on people doing. Most HLSL code that exists today isn't shared source with C++.

For the time being, I consider hacking in the tok::kw_const case in Parser::ParseTypeQualifierListOpt such that dxc accepts const qualifiers without ending up with a diagnostic. This approach seems to work for me.

It works by parsing the const qualifier, but it doesn't fix the underlying issues with overload resolution. Today our overload resolution ignores const-qualification, and we actually allow calling non-const methods on constant objects in some contexts because of hacks on top of hacks...

This PR however suggests that there is more work to do for proper const-qualifier support.

Fundamentally it is a change to the language grammar and syntax. All changes to language grammar should be gated on a language version and/or feature check.

If I know that at some point in the future const-methods will be supported, I would be more inclined to accept the hack mentioned above as a temporary measure.

Two points here:

(1) This isn't the place to talk about an implementation detail. This PR and the underlying proposal are about changing the language, not the compiler implementation.

(2) This hack would basically make it so that people writing code would write it in a way that gave them a reasonable expectation that it worked in one way, but it would actually work differently from expectations. That's the kind of behavior we're trying to remove from HLSL because we have too many instances of it.

I know it isn't satisfying, but the only answer today for people trying to share code between HLSL and C++ is to keep using preprocessor macros to morph the code between the languages. For this specific feature you'll need to wait until HLSL 202x begins development.

[ChristianReinbold]

@llvm-beanz

I know it isn't satisfying, but the only answer today...

Thanks for the detailed answer, which - for me - is perfectly satisfying. My intention when writing this comment was about managing expectations, and in this regard it helped a lot. I now have a better idea which rabbit hole we may continue to go down with the current code-base.

Our team is very small and our focus has been on Shader Model 6.8 rather than HLSL 202x over the last year.

I highly appreciate the work you are putting into HLSL, and I am genuinely impressed how well the feature set of HLSL 2021 worked out in practice so far, for a multitude of reasons. I guess we are just testing the limits with our use case. I hope you are fine with me inquiring here and there about features I would be interested in. The current approach of peeking into HLSL specs and dxc sources whenever struggling with unexpected behavior or compiler bugs / limitations, "fixing" them and upstreaming them whenever possible seems to work quite well. As long as you are happy to spend some time with irregular PRs that have the risk of not accounting for the whole picture and thus heading the wrong way sometimes (in the end my compiler background is limited to non-existant), I probably will continue with this approach.

(1) This isn't the place to talk about an implementation detail.

I am aware. I intended to give you a more complete picture about the implications this language feature has for us. I am sorry for having side-tracked the conversation with that.

(2) This hack would basically make it so that people writing code would write it in a way that gave them a reasonable expectation that it worked in one way, but it would actually work differently from expectations.

I agree. I do not consider bringing in this hack as a PR to the dxc repo.